diff mbox series

sysctl: fix proc_dobool() usability

Message ID 20230210145823.756906-1-omosnace@redhat.com (mailing list archive)
State New, archived
Headers show
Series sysctl: fix proc_dobool() usability | expand

Commit Message

Ondrej Mosnacek Feb. 10, 2023, 2:58 p.m. UTC
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(-)

Comments

Ondrej Mosnacek Feb. 20, 2023, 12:52 p.m. UTC | #1
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/
Kees Cook Feb. 21, 2023, 5:34 p.m. UTC | #2
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.
Luis Chamberlain Feb. 21, 2023, 9:36 p.m. UTC | #3
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
Ondrej Mosnacek Feb. 22, 2023, 7:52 a.m. UTC | #4
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!
Thomas Weißschuh March 5, 2023, 2:18 a.m. UTC | #5
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/
Storm Dragon March 5, 2023, 2:51 a.m. UTC | #6
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
Thomas Weißschuh March 5, 2023, 3:06 a.m. UTC | #7
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?
Storm Dragon March 6, 2023, 5:47 p.m. UTC | #8
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 mbox series

Patch

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,
 	},