Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220522052624.21493-1-songmuchun@bytedance.com> <202205231233.EE3AB926@keescook>
In-Reply-To: <202205231233.EE3AB926@keescook>
From:   Muchun Song <songmuchun@bytedance.com>
Date:   Tue, 24 May 2022 10:41:20 +0800
Message-ID: <CAMZfGtWw6bnpDcdjgQnsW8MMPy0BmHTtHdnX4codMdqk1b5wJg@mail.gmail.com>
Subject: Re: [PATCH v2] sysctl: handle table->maxlen properly for proc_dobool
To:     Kees Cook <keescook@chromium.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Matthew Wilcox <willy@infradead.org>,
        Luis Chamberlain <mcgrof@kernel.org>,
        Iurii Zaikin <yzaikin@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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/