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 <[email protected]>
---
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(-)
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,
},
--
2.39.1
On Fri, Feb 10, 2023 at 3:58 PM Ondrej Mosnacek <[email protected]> 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 <[email protected]>
> ---
> 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/
--
Ondrej Mosnacek
Senior Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
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 <[email protected]>
Ah nice, thanks for tracking this down.
Acked-by: Kees Cook <[email protected]>
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.
--
Kees Cook
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 <[email protected]>
>
> Ah nice, thanks for tracking this down.
>
> Acked-by: Kees Cook <[email protected]>
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 <[email protected]> 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 <[email protected]>
> >
> > Ah nice, thanks for tracking this down.
> >
> > Acked-by: Kees Cook <[email protected]>
>
> Queued onto sysctl-next, will send to Linus as this is a fix too.
Thanks, Luis!
--
Ondrej Mosnacek
Senior Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
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 <[email protected]>
Link: https://lore.kernel.org/lkml/[email protected]/
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 <[email protected]>
>Link: https://lore.kernel.org/lkml/[email protected]/
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 <[email protected]>
> > Link: https://lore.kernel.org/lkml/[email protected]/
>
> 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