[PATCH v2] sysctl: handle table->maxlen properly for proc_dobool

[PATCH v2] sysctl: handle table->maxlen properly for proc_dobool

[Muchun Song]

Setting ->proc_handler to proc_dobool at the same time setting ->maxlen
to sizeof(int) is counter-intuitive, it is easy to make mistakes.  For
robustness, fix it by reimplementing proc_dobool() properly.

Signed-off-by: Muchun Song <songmuchun@bytedance.com>
Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Iurii Zaikin <yzaikin@google.com>
---
v2:
 - Reimplementing proc_dobool().

 fs/lockd/svc.c  |  2 +-
 kernel/sysctl.c | 38 +++++++++++++++++++-------------------
 2 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index 59ef8a1f843f..6e48ee787f49 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(nsm_use_hostnames),
 		.mode		= 0644,
 		.proc_handler	= proc_dobool,
 	},
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index e52b6e372c60..50a2c29efc94 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -423,21 +423,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)
@@ -708,16 +693,31 @@ 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 up to table->maxlen/sizeof(bool) bool values from/to
+ * the user buffer, treated as an ASCII string.
  *
  * 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 = *table;
+	bool *data = table->data;
+	unsigned int val = READ_ONCE(*data);
+	int ret;
+
+	/* Do not support arrays yet. */
+	if (table->maxlen != sizeof(bool))
+		return -EINVAL;
+
+	tmp.maxlen = sizeof(val);
+	tmp.data = &val;
+	ret = do_proc_douintvec(&tmp, write, buffer, lenp, ppos, NULL, NULL);
+	if (ret)
+		return ret;
+	if (write)
+		WRITE_ONCE(*data, val ? true : false);
+	return 0;
 }
 
 /**
-- 
2.11.0

Re: [PATCH v2] sysctl: handle table->maxlen properly for proc_dobool

[Luis Chamberlain]

On Sun, May 22, 2022 at 01:26:24PM +0800, Muchun Song wrote:
> Setting ->proc_handler to proc_dobool at the same time setting ->maxlen
> to sizeof(int) is counter-intuitive, it is easy to make mistakes.  For
> robustness, fix it by reimplementing proc_dobool() properly.
> 
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> Cc: Luis Chamberlain <mcgrof@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Iurii Zaikin <yzaikin@google.com>
> ---

Thanks for your patch Muchun!

Does this fix an actualy issue? Because the comit log suggest so.
If so is there a bug which is known or a reproducer which can be
implemented to showcase that bug?

The reason I ask is that we have automatic scrapers for bug fixes,
and I tend to prefer to avoid giving those automatic scrapers
the idea that a patch is a fix for a kernel bug when it it is not.
If what you are change is an optimization then your commit log should
clarify that.

If you are fixing something then you must be clear about about the
details I mentioned. And then, if it does fix an issue, how long
has the issue been know, what are the consequences of it? And up
to what kernel is this issue present for?

  Luis

Re: [PATCH v2] sysctl: handle table->maxlen properly for proc_dobool

[Kees Cook]

On Sun, May 22, 2022 at 01:26:24PM +0800, Muchun Song wrote:
> Setting ->proc_handler to proc_dobool at the same time setting ->maxlen
> to sizeof(int) is counter-intuitive, it is easy to make mistakes.  For
> robustness, fix it by reimplementing proc_dobool() properly.
> 
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> Cc: Luis Chamberlain <mcgrof@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Iurii Zaikin <yzaikin@google.com>
> ---
> v2:
>  - Reimplementing proc_dobool().
> 
>  fs/lockd/svc.c  |  2 +-
>  kernel/sysctl.c | 38 +++++++++++++++++++-------------------
>  2 files changed, 20 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
> index 59ef8a1f843f..6e48ee787f49 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(nsm_use_hostnames),
>  		.mode		= 0644,
>  		.proc_handler	= proc_dobool,
>  	},

This hunk is fine -- it's a reasonable fix-up.

> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index e52b6e372c60..50a2c29efc94 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -423,21 +423,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)
> @@ -708,16 +693,31 @@ 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 up to table->maxlen/sizeof(bool) bool values from/to
> + * the user buffer, treated as an ASCII string.
>   *
>   * 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 = *table;
> +	bool *data = table->data;
> +	unsigned int val = READ_ONCE(*data);
> +	int ret;
> +
> +	/* Do not support arrays yet. */
> +	if (table->maxlen != sizeof(bool))
> +		return -EINVAL;
> +
> +	tmp.maxlen = sizeof(val);
> +	tmp.data = &val;
> +	ret = do_proc_douintvec(&tmp, write, buffer, lenp, ppos, NULL, NULL);
> +	if (ret)
> +		return ret;
> +	if (write)
> +		WRITE_ONCE(*data, val ? true : false);
> +	return 0;
>  }

This part I don't understand -- it just inlines do_proc_dobool_conv(),
and I think detracts from readability.

-- 
Kees Cook

Re: [PATCH v2] sysctl: handle table->maxlen properly for proc_dobool

[Muchun Song]

On Tue, May 24, 2022 at 1:27 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Sun, May 22, 2022 at 01:26:24PM +0800, Muchun Song wrote:
> > Setting ->proc_handler to proc_dobool at the same time setting ->maxlen
> > to sizeof(int) is counter-intuitive, it is easy to make mistakes.  For
> > robustness, fix it by reimplementing proc_dobool() properly.
> >
> > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > Cc: Luis Chamberlain <mcgrof@kernel.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Iurii Zaikin <yzaikin@google.com>
> > ---
>
> Thanks for your patch Muchun!
>
> Does this fix an actualy issue? Because the comit log suggest so.

Thanks for taking a look.

I think it is an improvement not a real bug fix. When I first use
proc_dobool in my driver, I assign sizeof(variable) to table->maxlen.
Then I found it was wrong, it should be sizeof(int) which was
counter-intuitive. So it is very easy to make mistakes. Should I add
those into the commit log?

Thanks.

> If so is there a bug which is known or a reproducer which can be
> implemented to showcase that bug?
>
> The reason I ask is that we have automatic scrapers for bug fixes,
> and I tend to prefer to avoid giving those automatic scrapers
> the idea that a patch is a fix for a kernel bug when it it is not.
> If what you are change is an optimization then your commit log should
> clarify that.
>
> If you are fixing something then you must be clear about about the
> details I mentioned. And then, if it does fix an issue, how long
> has the issue been know, what are the consequences of it? And up
> to what kernel is this issue present for?
>
>   Luis

Re: [PATCH v2] sysctl: handle table->maxlen properly for proc_dobool

[Muchun Song]

On Tue, May 24, 2022 at 3:33 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Sun, May 22, 2022 at 01:26:24PM +0800, Muchun Song wrote:
> > Setting ->proc_handler to proc_dobool at the same time setting ->maxlen
> > to sizeof(int) is counter-intuitive, it is easy to make mistakes.  For
> > robustness, fix it by reimplementing proc_dobool() properly.
> >
> > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > Cc: Luis Chamberlain <mcgrof@kernel.org>
> > Cc: Kees Cook <keescook@chromium.org>
> > Cc: Iurii Zaikin <yzaikin@google.com>
> > ---
> > v2:
> >  - Reimplementing proc_dobool().
> >
> >  fs/lockd/svc.c  |  2 +-
> >  kernel/sysctl.c | 38 +++++++++++++++++++-------------------
> >  2 files changed, 20 insertions(+), 20 deletions(-)
> >
> > diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
> > index 59ef8a1f843f..6e48ee787f49 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(nsm_use_hostnames),
> >               .mode           = 0644,
> >               .proc_handler   = proc_dobool,
> >       },
>
> This hunk is fine -- it's a reasonable fix-up.
>
> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> > index e52b6e372c60..50a2c29efc94 100644
> > --- a/kernel/sysctl.c
> > +++ b/kernel/sysctl.c
> > @@ -423,21 +423,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)
> > @@ -708,16 +693,31 @@ 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 up to table->maxlen/sizeof(bool) bool values from/to
> > + * the user buffer, treated as an ASCII string.
> >   *
> >   * 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 = *table;
> > +     bool *data = table->data;
> > +     unsigned int val = READ_ONCE(*data);
> > +     int ret;
> > +
> > +     /* Do not support arrays yet. */
> > +     if (table->maxlen != sizeof(bool))
> > +             return -EINVAL;
> > +
> > +     tmp.maxlen = sizeof(val);
> > +     tmp.data = &val;
> > +     ret = do_proc_douintvec(&tmp, write, buffer, lenp, ppos, NULL, NULL);
> > +     if (ret)
> > +             return ret;
> > +     if (write)
> > +             WRITE_ONCE(*data, val ? true : false);
> > +     return 0;
> >  }
>
> This part I don't understand -- it just inlines do_proc_dobool_conv(),
> and I think detracts from readability.
>

I think do_proc_dobool_conv() is an abuse of do_proc_dointvec()
since do_proc_dointvec() expects a "int" type data instead of a "bool".
As you can see, there is some cast from bool to int or int to bool
in do_proc_dobool_conv().  And do_proc_dobool_conv() supports
arrays, while proc_dobool() does not support. It is a little ugly to
fix this in __do_proc_dointvec() (I have fixed it in v1 [1]).

This version refers to proc_dou8vec_minmax(). For me, I think it
is cleaner than v1, any thoughts?

[1] https://lore.kernel.org/all/20220519125505.92400-1-songmuchun@bytedance.com/

Re: [PATCH v2] sysctl: handle table->maxlen properly for proc_dobool

[Luis Chamberlain]

On Tue, May 24, 2022 at 10:30:01AM +0800, Muchun Song wrote:
> On Tue, May 24, 2022 at 1:27 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
> >
> > On Sun, May 22, 2022 at 01:26:24PM +0800, Muchun Song wrote:
> > > Setting ->proc_handler to proc_dobool at the same time setting ->maxlen
> > > to sizeof(int) is counter-intuitive, it is easy to make mistakes.  For
> > > robustness, fix it by reimplementing proc_dobool() properly.
> > >
> > > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > > Cc: Luis Chamberlain <mcgrof@kernel.org>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Cc: Iurii Zaikin <yzaikin@google.com>
> > > ---
> >
> > Thanks for your patch Muchun!
> >
> > Does this fix an actualy issue? Because the comit log suggest so.
> 
> Thanks for taking a look.
> 
> I think it is an improvement not a real bug fix.

Then please adjust the commit log accordingly.

> When I first use
> proc_dobool in my driver, I assign sizeof(variable) to table->maxlen.
> Then I found it was wrong, it should be sizeof(int) which was
> counter-intuitive. So it is very easy to make mistakes. Should I add
> those into the commit log?

Yes that is useful information when doing patch review as well.

  Luis

Re: [PATCH v2] sysctl: handle table->maxlen properly for proc_dobool

[Muchun Song]

On Wed, May 25, 2022 at 12:20 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Tue, May 24, 2022 at 10:30:01AM +0800, Muchun Song wrote:
> > On Tue, May 24, 2022 at 1:27 AM Luis Chamberlain <mcgrof@kernel.org> wrote:
> > >
> > > On Sun, May 22, 2022 at 01:26:24PM +0800, Muchun Song wrote:
> > > > Setting ->proc_handler to proc_dobool at the same time setting ->maxlen
> > > > to sizeof(int) is counter-intuitive, it is easy to make mistakes.  For
> > > > robustness, fix it by reimplementing proc_dobool() properly.
> > > >
> > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > > > Cc: Luis Chamberlain <mcgrof@kernel.org>
> > > > Cc: Kees Cook <keescook@chromium.org>
> > > > Cc: Iurii Zaikin <yzaikin@google.com>
> > > > ---
> > >
> > > Thanks for your patch Muchun!
> > >
> > > Does this fix an actualy issue? Because the comit log suggest so.
> >
> > Thanks for taking a look.
> >
> > I think it is an improvement not a real bug fix.
>
> Then please adjust the commit log accordingly.
>
> > When I first use
> > proc_dobool in my driver, I assign sizeof(variable) to table->maxlen.
> > Then I found it was wrong, it should be sizeof(int) which was
> > counter-intuitive. So it is very easy to make mistakes. Should I add
> > those into the commit log?
>
> Yes that is useful information when doing patch review as well.
>

Thanks. I'll update in v3.