Re: [PATCH v3] sysctl: handle table->maxlen robustly for proc_dobool

[Muchun Song]

Hi all,

Ping guys. Any comments or objections?

On Wed, May 25, 2022 at 2:51 PM Muchun Song <songmuchun@xxxxxxxxxxxxx> 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 in the
> future (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 very counter-intuitive).  For robustness,
> rework proc_dobool() robustly.  So it is an improvement not a real bug
> fix.
>
> Signed-off-by: Muchun Song <songmuchun@xxxxxxxxxxxxx>
> Cc: Luis Chamberlain <mcgrof@xxxxxxxxxx>
> Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> Cc: Iurii Zaikin <yzaikin@xxxxxxxxxx>
> ---
> v3:
>  - Update commit log.
>
> 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
>