From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 63398C433EF for ; Tue, 24 May 2022 02:42:04 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233381AbiEXCmC (ORCPT ); Mon, 23 May 2022 22:42:02 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48552 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232542AbiEXCl6 (ORCPT ); Mon, 23 May 2022 22:41:58 -0400 Received: from mail-yb1-xb34.google.com (mail-yb1-xb34.google.com [IPv6:2607:f8b0:4864:20::b34]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id AE22E369DF for ; Mon, 23 May 2022 19:41:56 -0700 (PDT) Received: by mail-yb1-xb34.google.com with SMTP id b124so16622046ybg.12 for ; Mon, 23 May 2022 19:41:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bytedance-com.20210112.gappssmtp.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=EZK/LIj/HmIKuwXlrjzuY0xVYI4ZQoH/y1QWOLzPiDs=; b=fDGfFSmM6IwKD9Bwy3Vbjs5q0zo++Yssf/Twwxbsj6rtaD4lf9KPR4hRKzRGxJu0Z/ GMTlLKofZv7ym+eDgCLHzWEx2+jjJn90uiGOvR6N6R8hHAVXOGXh+zQHUQ8zczlZvxzE Jm253Or7yrL5QuDlq4Z+X9b6RVaUwb1Fwi8WJPgQN9WmlwPEBI7d2NwdPRDtevU2waT4 cAP2AmgytQPM3j1J7VMxImjt4eklby19Y935kh5mrCDQOQ6wjGaP4lVHBAz+HIiqHmNj 4EX8W8rDQYo84FDGKlHRYh6P1mDySLZ5nutESL0v6ngM1G86xpJYGxIJo4WBDcl1RiVv jggA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=EZK/LIj/HmIKuwXlrjzuY0xVYI4ZQoH/y1QWOLzPiDs=; b=v6FgXH0bab3VT10VdKdgs1DaqsWOpiteNWKbcyWY+8+xte7amBuN5SFPKpDqbH2DtW BumoTjkrmc06SlBgVbt8QMGDJuicm2vQwFHWT8LX42o+5r9C21zH0srNxI/KdVD/0YIo r+0uJhk4iRAvLQXxSUu1Bjvq3bvyJiY+DVtO5iyrUONstI5DuwY9KgMJ0e+GiK0FgKA+ seReAfWhAICdBa9yTdRpl4dwIJ7AQEpccw9guOUeGWTA+60R1O6A+1pE6t+/1k69vnu3 NCfscPNbzRTRi0+JyY7gm0tdETy4GykmQYt5i9f2MFhTcpm3hQGDhcoacnhdk7cMrb19 /67A== X-Gm-Message-State: AOAM530hY6TgPVV7XGbOS60NNXLMvlbENTcFvyv8pj4dp+oHHNNjg4/t Ti28+7lXQkgw7TYtNVpoxBbqcYXKzMc/GSTx5ISMfuuIX7Q00Q== X-Google-Smtp-Source: ABdhPJyQiAgqee0mEcshKFUmX0Jszav+XGkGtIOWt3IFQhjkCvtAp6nxMN17Po35qVDTGKQ+LlLz81HpxbaQwQ+SUcQ= X-Received: by 2002:a25:3452:0:b0:64e:1776:ce90 with SMTP id b79-20020a253452000000b0064e1776ce90mr24814077yba.261.1653360116001; Mon, 23 May 2022 19:41:56 -0700 (PDT) MIME-Version: 1.0 References: <20220522052624.21493-1-songmuchun@bytedance.com> <202205231233.EE3AB926@keescook> In-Reply-To: <202205231233.EE3AB926@keescook> From: Muchun Song Date: Tue, 24 May 2022 10:41:20 +0800 Message-ID: Subject: Re: [PATCH v2] sysctl: handle table->maxlen properly for proc_dobool To: Kees Cook Cc: LKML , linux-fsdevel , Matthew Wilcox , Luis Chamberlain , Iurii Zaikin Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, May 24, 2022 at 3:33 AM Kees Cook 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 > > Cc: Luis Chamberlain > > Cc: Kees Cook > > Cc: Iurii Zaikin > > --- > > 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/