| Message ID | 20230210145823.756906-1-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sysctl: fix proc_dobool() usability | expand |
On Fri, Feb 10, 2023 at 3:58 PM Ondrej Mosnacek <omosnace@redhat.com> wrote: > Currently proc_dobool expects a (bool *) in table->data, but sizeof(int) > in table->maxsize, because it uses do_proc_dointvec() directly. > > This is unsafe for at least two reasons: > 1. A sysctl table definition may use { .data = &variable, .maxsize = > sizeof(variable) }, not realizing that this makes the sysctl unusable > (see the Fixes: tag) and that they need to use the completely > counterintuitive sizeof(int) instead. > 2. proc_dobool() will currently try to parse an array of values if given > .maxsize >= 2*sizeof(int), but will try to write values of type bool > by offsets of sizeof(int), so it will not work correctly with neither > an (int *) nor a (bool *). There is no .maxsize validation to prevent > this. > > Fix this by: > 1. Constraining proc_dobool() to allow only one value and .maxsize == > sizeof(bool). > 2. Wrapping the original struct ctl_table in a temporary one with .data > pointing to a local int variable and .maxsize set to sizeof(int) and > passing this one to proc_dointvec(), converting the value to/from > bool as needed (using proc_dou8vec_minmax() as an example). > 3. Extending sysctl_check_table() to enforce proc_dobool() expectations. > 4. Fixing the proc_dobool() docstring (it was just copy-pasted from > proc_douintvec, apparently...). > 5. Converting all existing proc_dobool() users to set .maxsize to > sizeof(bool) instead of sizeof(int). > > Fixes: 83efeeeb3d04 ("tty: Allow TIOCSTI to be disabled") > Fixes: a2071573d634 ("sysctl: introduce new proc handler proc_dobool") > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > --- > fs/lockd/svc.c | 2 +- > fs/proc/proc_sysctl.c | 6 ++++++ > kernel/sysctl.c | 43 ++++++++++++++++++++++++------------------- > mm/hugetlb_vmemmap.c | 2 +- > 4 files changed, 32 insertions(+), 21 deletions(-) Gentle ping... Without this patch the new "dev.tty.legacy_tiocsti" sysctl is unusable. This is blocking me from making selinux-testsuite work with CONFIG_LEGACY_TIOCSTI=n: https://lore.kernel.org/selinux/CAHC9VhQwrjwdW27+ktcT_9q-N7AmuUK8GYgoYbPXGVAcjwA4nQ@mail.gmail.com/T/
On Fri, Feb 10, 2023 at 03:58:23PM +0100, Ondrej Mosnacek wrote: > Currently proc_dobool expects a (bool *) in table->data, but sizeof(int) > in table->maxsize, because it uses do_proc_dointvec() directly. > > This is unsafe for at least two reasons: > 1. A sysctl table definition may use { .data = &variable, .maxsize = > sizeof(variable) }, not realizing that this makes the sysctl unusable > (see the Fixes: tag) and that they need to use the completely > counterintuitive sizeof(int) instead. > 2. proc_dobool() will currently try to parse an array of values if given > .maxsize >= 2*sizeof(int), but will try to write values of type bool > by offsets of sizeof(int), so it will not work correctly with neither > an (int *) nor a (bool *). There is no .maxsize validation to prevent > this. > > Fix this by: > 1. Constraining proc_dobool() to allow only one value and .maxsize == > sizeof(bool). > 2. Wrapping the original struct ctl_table in a temporary one with .data > pointing to a local int variable and .maxsize set to sizeof(int) and > passing this one to proc_dointvec(), converting the value to/from > bool as needed (using proc_dou8vec_minmax() as an example). > 3. Extending sysctl_check_table() to enforce proc_dobool() expectations. > 4. Fixing the proc_dobool() docstring (it was just copy-pasted from > proc_douintvec, apparently...). > 5. Converting all existing proc_dobool() users to set .maxsize to > sizeof(bool) instead of sizeof(int). > > Fixes: 83efeeeb3d04 ("tty: Allow TIOCSTI to be disabled") > Fixes: a2071573d634 ("sysctl: introduce new proc handler proc_dobool") > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> Ah nice, thanks for tracking this down. Acked-by: Kees Cook <keescook@chromium.org> I've long wanted to replace all the open-coded sysctl initializers with a helper macro so it's hard to make mistakes. e.g. #define _SYSCTL_HANDLER(var) \ _Generic((var), \ default: proc_dointvec_minmax, \ unsigned int: proc_douintvec_minmax, \ unsigned long: proc_doulongvec_minmax, \ char: proc_dou8vec_minmax, \ bool: proc_dobool, \ char *: proc_dostring) #define _SYSCTL_MINVAL(var) \ _Generic((var), \ default: 0, \ int: INT_MIN, \ unsigned int: UINT_MIN, \ unsigned long: ULONG_MIN, \ char: U8_MIN, \ bool: 0) #define _SYSCTL_MAXVAL(var) \ _Generic((var), \ default: 0, \ int: INT_MAX, \ unsigned int: UINT_MAX, \ unsigned long: ULONG_MAX, \ char: U8_MAX, \ bool: 1) #define _SYSCTL_VAR(_name, _var, _mode, _handler, _len, _min, _max) \ { \ .procname = _name, \ .data = &(_var), \ .maxlen = _len, \ .mode = _mode, \ .proc_handler = _handler, \ .minlen = _min, \ .maxlen = _min, \ } /* single value */ #define SYSCTL_VAL(var) \ _SYSCTL_VAR(#var, var, 0644, _SYSCTL_HANDLER(var), sizeof(var), \ _SYSCTL_MINVAL(var), _SYSCTL_MAXVAL(var)) /* single value with min/max */ #define SYSCTL_VAL_MINMAX(_var, _min, _max) \ _SYSCTL_VAR(#var, var, 0644, _SYSCTL_HANDLER(var), sizeof(var), \ _min, _max) /* Strings... */ #define SYSCTL_STR(_var, _len) \ _SYSCTL_VAR(#var, var, 0644, _SYSCTL_HANDLER(var), _len, 0, 0) /* Arrays... */ #define SYSCTL_ARRAY(_var) \ _SYSCTL_VAR(#var, var, 0644, _SYSCTL_HANDLER(var), \ ARRAY_SIZE(_var), _SYSCTL_MINVAL(var), \ _SYSCTL_MAXVAL(var) Totally untested. I think this would cover the vast majority of sysctl initializers.
On Tue, Feb 21, 2023 at 09:34:49AM -0800, Kees Cook wrote: > On Fri, Feb 10, 2023 at 03:58:23PM +0100, Ondrej Mosnacek wrote: > > Currently proc_dobool expects a (bool *) in table->data, but sizeof(int) > > in table->maxsize, because it uses do_proc_dointvec() directly. > > > > This is unsafe for at least two reasons: > > 1. A sysctl table definition may use { .data = &variable, .maxsize = > > sizeof(variable) }, not realizing that this makes the sysctl unusable > > (see the Fixes: tag) and that they need to use the completely > > counterintuitive sizeof(int) instead. > > 2. proc_dobool() will currently try to parse an array of values if given > > .maxsize >= 2*sizeof(int), but will try to write values of type bool > > by offsets of sizeof(int), so it will not work correctly with neither > > an (int *) nor a (bool *). There is no .maxsize validation to prevent > > this. > > > > Fix this by: > > 1. Constraining proc_dobool() to allow only one value and .maxsize == > > sizeof(bool). > > 2. Wrapping the original struct ctl_table in a temporary one with .data > > pointing to a local int variable and .maxsize set to sizeof(int) and > > passing this one to proc_dointvec(), converting the value to/from > > bool as needed (using proc_dou8vec_minmax() as an example). > > 3. Extending sysctl_check_table() to enforce proc_dobool() expectations. > > 4. Fixing the proc_dobool() docstring (it was just copy-pasted from > > proc_douintvec, apparently...). > > 5. Converting all existing proc_dobool() users to set .maxsize to > > sizeof(bool) instead of sizeof(int). > > > > Fixes: 83efeeeb3d04 ("tty: Allow TIOCSTI to be disabled") > > Fixes: a2071573d634 ("sysctl: introduce new proc handler proc_dobool") > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > > Ah nice, thanks for tracking this down. > > Acked-by: Kees Cook <keescook@chromium.org> Queued onto sysctl-next, will send to Linus as this is a fix too. Luis
On Tue, Feb 21, 2023 at 11:04 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > On Tue, Feb 21, 2023 at 09:34:49AM -0800, Kees Cook wrote: > > On Fri, Feb 10, 2023 at 03:58:23PM +0100, Ondrej Mosnacek wrote: > > > Currently proc_dobool expects a (bool *) in table->data, but sizeof(int) > > > in table->maxsize, because it uses do_proc_dointvec() directly. > > > > > > This is unsafe for at least two reasons: > > > 1. A sysctl table definition may use { .data = &variable, .maxsize = > > > sizeof(variable) }, not realizing that this makes the sysctl unusable > > > (see the Fixes: tag) and that they need to use the completely > > > counterintuitive sizeof(int) instead. > > > 2. proc_dobool() will currently try to parse an array of values if given > > > .maxsize >= 2*sizeof(int), but will try to write values of type bool > > > by offsets of sizeof(int), so it will not work correctly with neither > > > an (int *) nor a (bool *). There is no .maxsize validation to prevent > > > this. > > > > > > Fix this by: > > > 1. Constraining proc_dobool() to allow only one value and .maxsize == > > > sizeof(bool). > > > 2. Wrapping the original struct ctl_table in a temporary one with .data > > > pointing to a local int variable and .maxsize set to sizeof(int) and > > > passing this one to proc_dointvec(), converting the value to/from > > > bool as needed (using proc_dou8vec_minmax() as an example). > > > 3. Extending sysctl_check_table() to enforce proc_dobool() expectations. > > > 4. Fixing the proc_dobool() docstring (it was just copy-pasted from > > > proc_douintvec, apparently...). > > > 5. Converting all existing proc_dobool() users to set .maxsize to > > > sizeof(bool) instead of sizeof(int). > > > > > > Fixes: 83efeeeb3d04 ("tty: Allow TIOCSTI to be disabled") > > > Fixes: a2071573d634 ("sysctl: introduce new proc handler proc_dobool") > > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> > > > > Ah nice, thanks for tracking this down. > > > > Acked-by: Kees Cook <keescook@chromium.org> > > Queued onto sysctl-next, will send to Linus as this is a fix too. Thanks, Luis!
Hi -stable team,
please backport the commit f1aa2eb5ea05 ("sysctl: fix proc_dobool() usability")
to the stable kernels containing commit 83efeeeb3d04 ("tty: Allow TIOCSTI to be disabled").
(Which seems only to be the 6.2 branch only at the moment)
Without this backport the sysctl dev.net.legacy_tiocsti to enable
ioctl(TIOCSTI) is not functional. So on kernels that don't enable
CONFIG_LEGACY_TIOCSTI, ioctl(TIOCSTI) is not usable at all.
This ioctl is used for the copy-and-paste functionality of the
screenreader "fenrir".
( https://github.com/chrys87/fenrir )
Reported-by: Storm Dragon <stormdragon2976@gmail.com>
Link: https://lore.kernel.org/lkml/ZAOi9hDBTYqoAZuI@hotmail.com/
On Sun, Mar 05, 2023 at 02:18:11AM +0000, Thomas Weißschuh wrote: >This ioctl is used for the copy-and-paste functionality of the >screenreader "fenrir". >( https://github.com/chrys87/fenrir ) > >Reported-by: Storm Dragon <stormdragon2976@gmail.com> >Link: https://lore.kernel.org/lkml/ZAOi9hDBTYqoAZuI@hotmail.com/ I believe this will also cause some loss of functionality in brltty as well: https://brltty.app
On Sat, Mar 04, 2023 at 09:51:49PM -0500, Storm Dragon wrote: > On Sun, Mar 05, 2023 at 02:18:11AM +0000, Thomas Weißschuh wrote: > > This ioctl is used for the copy-and-paste functionality of the > > screenreader "fenrir". > > ( https://github.com/chrys87/fenrir ) > > > > Reported-by: Storm Dragon <stormdragon2976@gmail.com> > > Link: https://lore.kernel.org/lkml/ZAOi9hDBTYqoAZuI@hotmail.com/ > > I believe this will also cause some loss of functionality in brltty as > well: > > https://brltty.app The documentation of brltty indicates that they only use TIOCSTI as fallback. By default a virtual keyboard device is used to simulate typing. Maybe it would also make sense to open a ticket to ArchLinux to enable CONFIG_LEGACY_TIOCSTI again, as per the kernel default. In accordance with the options help text: "Say 'Y here only if you have confirmed that yout system's userspace depends on this functionality to continue operating normally" Could you create such a ticket if think it's necessary?
On Sun, Mar 05, 2023 at 03:06:12AM +0000, Thomas Weißschuh wrote: >Maybe it would also make sense to open a ticket to ArchLinux to enable >CONFIG_LEGACY_TIOCSTI again, as per the kernel default. > >In accordance with the options help text: > >"Say 'Y here only if you have confirmed that yout system's userspace >depends on this functionality to continue operating normally" > >Could you create such a ticket if think it's necessary? The ticket has been created. The link is: https://bugs.archlinux.org/task/77745 Thanks, Storm
diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c index 59ef8a1f843f3..914ea1c3537d1 100644 --- a/fs/lockd/svc.c +++ b/fs/lockd/svc.c @@ -496,7 +496,7 @@ static struct ctl_table nlm_sysctls[] = { { .procname = "nsm_use_hostnames", .data = &nsm_use_hostnames, - .maxlen = sizeof(int), + .maxlen = sizeof(bool), .mode = 0644, .proc_handler = proc_dobool, }, diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c index 48f2d60bd78a2..436025e0f77a6 100644 --- a/fs/proc/proc_sysctl.c +++ b/fs/proc/proc_sysctl.c @@ -1124,6 +1124,11 @@ static int sysctl_check_table_array(const char *path, struct ctl_table *table) err |= sysctl_err(path, table, "array not allowed"); } + if (table->proc_handler == proc_dobool) { + if (table->maxlen != sizeof(bool)) + err |= sysctl_err(path, table, "array not allowed"); + } + return err; } @@ -1136,6 +1141,7 @@ static int sysctl_check_table(const char *path, struct ctl_table *table) err |= sysctl_err(path, entry, "Not a file"); if ((entry->proc_handler == proc_dostring) || + (entry->proc_handler == proc_dobool) || (entry->proc_handler == proc_dointvec) || (entry->proc_handler == proc_douintvec) || (entry->proc_handler == proc_douintvec_minmax) || diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 137d4abe3eda1..1c240d2c99bcb 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -425,21 +425,6 @@ static void proc_put_char(void **buf, size_t *size, char c) } } -static int do_proc_dobool_conv(bool *negp, unsigned long *lvalp, - int *valp, - int write, void *data) -{ - if (write) { - *(bool *)valp = *lvalp; - } else { - int val = *(bool *)valp; - - *lvalp = (unsigned long)val; - *negp = false; - } - return 0; -} - static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp, int *valp, int write, void *data) @@ -710,16 +695,36 @@ int do_proc_douintvec(struct ctl_table *table, int write, * @lenp: the size of the user buffer * @ppos: file position * - * Reads/writes up to table->maxlen/sizeof(unsigned int) integer - * values from/to the user buffer, treated as an ASCII string. + * Reads/writes one integer value from/to the user buffer, + * treated as an ASCII string. + * + * table->data must point to a bool variable and table->maxlen must + * be sizeof(bool). * * Returns 0 on success. */ int proc_dobool(struct ctl_table *table, int write, void *buffer, size_t *lenp, loff_t *ppos) { - return do_proc_dointvec(table, write, buffer, lenp, ppos, - do_proc_dobool_conv, NULL); + struct ctl_table tmp; + bool *data = table->data; + int res, val; + + /* Do not support arrays yet. */ + if (table->maxlen != sizeof(bool)) + return -EINVAL; + + tmp = *table; + tmp.maxlen = sizeof(val); + tmp.data = &val; + + val = READ_ONCE(*data); + res = proc_dointvec(&tmp, write, buffer, lenp, ppos); + if (res) + return res; + if (write) + WRITE_ONCE(*data, val); + return 0; } /** diff --git a/mm/hugetlb_vmemmap.c b/mm/hugetlb_vmemmap.c index 45e93a545dd7e..a559037cce00c 100644 --- a/mm/hugetlb_vmemmap.c +++ b/mm/hugetlb_vmemmap.c @@ -581,7 +581,7 @@ static struct ctl_table hugetlb_vmemmap_sysctls[] = { { .procname = "hugetlb_optimize_vmemmap", .data = &vmemmap_optimize_enabled, - .maxlen = sizeof(int), + .maxlen = sizeof(vmemmap_optimize_enabled), .mode = 0644, .proc_handler = proc_dobool, },
Currently proc_dobool expects a (bool *) in table->data, but sizeof(int) in table->maxsize, because it uses do_proc_dointvec() directly. This is unsafe for at least two reasons: 1. A sysctl table definition may use { .data = &variable, .maxsize = sizeof(variable) }, not realizing that this makes the sysctl unusable (see the Fixes: tag) and that they need to use the completely counterintuitive sizeof(int) instead. 2. proc_dobool() will currently try to parse an array of values if given .maxsize >= 2*sizeof(int), but will try to write values of type bool by offsets of sizeof(int), so it will not work correctly with neither an (int *) nor a (bool *). There is no .maxsize validation to prevent this. Fix this by: 1. Constraining proc_dobool() to allow only one value and .maxsize == sizeof(bool). 2. Wrapping the original struct ctl_table in a temporary one with .data pointing to a local int variable and .maxsize set to sizeof(int) and passing this one to proc_dointvec(), converting the value to/from bool as needed (using proc_dou8vec_minmax() as an example). 3. Extending sysctl_check_table() to enforce proc_dobool() expectations. 4. Fixing the proc_dobool() docstring (it was just copy-pasted from proc_douintvec, apparently...). 5. Converting all existing proc_dobool() users to set .maxsize to sizeof(bool) instead of sizeof(int). Fixes: 83efeeeb3d04 ("tty: Allow TIOCSTI to be disabled") Fixes: a2071573d634 ("sysctl: introduce new proc handler proc_dobool") Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com> --- fs/lockd/svc.c | 2 +- fs/proc/proc_sysctl.c | 6 ++++++ kernel/sysctl.c | 43 ++++++++++++++++++++++++------------------- mm/hugetlb_vmemmap.c | 2 +- 4 files changed, 32 insertions(+), 21 deletions(-)