From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-174.mta0.migadu.com (out-174.mta0.migadu.com [91.218.175.174]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id DF74A16415 for ; Sun, 19 Jan 2025 15:00:21 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=91.218.175.174 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737298824; cv=none; b=sZpd4nGEp5MtICeUXimKJBq5/X1bnPZnqycvR4oYJoLxG0+0A60UZY0wPMDUdOcJMjjEcruZjkP5sxVGZQpX+YZ2kqTcNLHWn1f22WeK5cudrMY+fHyd+PjawFsLKwH4tJUOk5ungVRGqV2lCwFlQ5swCqd8oga1UnX6oPPWaWo= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737298824; c=relaxed/simple; bh=p7WjAWPWSybHzcGQXBYh/4o630mBC6FhXcTonewaa7Y=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=tUcCTIxSH7Zvn6ae6w3QHIK8veEGHfDpJ05uuDOtSPHlhcSUadiEjIFUqCBFDOJvCfJDsXpwvpLHphDqc8crpDtvgGYi7/0lU3C8CNJsEm52ljlNx2nTodXTNIIX5eYFONg/WNYg311aptH+kMWXMGXXKuRhUZsw+6cnWoOk+lE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=QB4dqxm3; arc=none smtp.client-ip=91.218.175.174 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="QB4dqxm3" Message-ID: <58da9dcb-a4ea-4d23-a7e5-b7f92293831a@linux.dev> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1737298814; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=N//chJ9dc7+2KDAlwT8w+6LXwkasBnM+E1rmDO1FjxA=; b=QB4dqxm3DLIzcL2NLKNYWRpMv6heLNAmOp0jGOHxF6VJloxl7/TaGG2zTGOQ1S2fkF3BrI KIwP6UDQ2421wmssh5MI6nl8ycPMLWILzby3nCew5CsZRa90mNor3Xoz26j3nlxc9zgIog NUddO0n1sG9UPhIuW4hSVIKsH0H5SlA= Date: Sun, 19 Jan 2025 22:59:21 +0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Subject: Re: [PATCH v5] sysctl: simplify the min/max boundary check To: Joel Granados Cc: "Eric W . Biederman" , Luis Chamberlain , Kees Cook , Christian Brauner , Dave Young , linux-kernel@vger.kernel.org References: <20250105152853.211037-1-wen.yang@linux.dev> Content-Language: en-US X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Wen Yang In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Migadu-Flow: FLOW_OUT On 2025/1/16 17:37, Joel Granados wrote: > On Sun, Jan 05, 2025 at 11:28:53PM +0800, Wen Yang wrote: >> do_proc_dointvec_conv() used the default range of int type, while >> do_proc_dointvec_minmax_conv() additionally used "int * {min, max}" of >> struct do_proc_dointvec_minmax_conv_param, which are actually passed >> in table->extra{1,2} pointers. >> >> By changing the struct do_proc_dointvec_minmax_conv_param's >> "int * {min, max}" to "int {min, max}" and setting the min/max value >> via dereference the table->extra{1, 2}, those do_proc_dointvec_conv() and >> do_proc_dointvec_minmax_conv() functions can be merged into one and reduce >> code by one-third. >> >> Similar changes were also made for do_proc_douintvec_minmax_conv_param, >> do_proc_douintvec_conv() and do_proc_douintvec_minmax_conv(). >> > Before moving forward, I need the motivation. > > Please answer this: > *Why* are you pushing these two [1], [2] series? > > Best > > [1] > * https://lore.kernel.org/tencent_A7FD9E9E75AA74A87583A427F5D20F988B09@qq.com > * https://lore.kernel.org/tencent_C5E6023F97E7CC2A046AAEA09BC9ACF43907@qq.com > * https://lore.kernel.org/cover.1726365007.git.wen.yang@linux.dev > * https://lore.kernel.org/cover.1726910671.git.wen.yang@linux.dev > > [2] > * https://lore.kernel.org/20240714173858.52163-1-wen.yang@linux.dev > * https://lore.kernel.org/20240715150223.54707-1-wen.yang@linux.dev > * https://lore.kernel.org/20240905134818.4104-1-wen.yang@linux.dev > * https://lore.kernel.org/20241201140058.5653-1-wen.yang@linux.dev > Thank you for your comments. Here is a brief report about it. The boundary check of sysctl uses .extra{1, 2} pointers, which need to point to constant variables (such as two_five_five, n_65535, ue_int_max, etc. that appear in multiple modules). We first try to stuff these variables into the shared const arrays ( (sysctl_vals, sysctl_long_vals) to save memory, as shown in series 1. Thank Eric for pointing out: - You can still save a lot more by turning .extra1 and .extra2 - into longs instead of keeping them as pointers and needing - constants to be pointed at somewhere. We have further found more detailed information about "kill the shared const array": https://lkml.kernel.org/87zgpte9o4.fsf@email.froward.int.ebiederm.org 67ff32289 ("proc_sysctl: update docs for __register_sysctl_table()") So we tried some ways to achieve it, as shown in series 2. Step 1: The extra{1,2} pointer is used as {min, max} pointers in the do_proc_do{int, uint}vec_minmax_conv_param structures. By changing the {min, max} pointers to numeric values, the code is simplified and preparations are made for the second step. Step 2: Add some sysctl helper functions to avoid direct access to table->extra1/extra2, and also support encoding values directly in the table entry. https://lore.kernel.org/all/0f4a5233b75e6484a6236d85d2b506c96ea41ef1.1726910671.git.wen.yang@linux.dev/ https://lore.kernel.org/all/a086609632328c2feee69b10067e43115885b2ae.1726910671.git.wen.yang@linux.dev/ Step 3: gradually remove the extra constant variables in multiple modules. https://lore.kernel.org/all/5f753895a5f31cac65ddeb56b58fc17553785163.1726910671.git.wen.yang@linux.dev/ https://lore.kernel.org/all/20d67551ed7fa9c774d2b128ad9bc298a0a55c9d.1726910671.git.wen.yang@linux.dev/ -- Best wishes, Wen >> Selftest were also made: >> >> [23:16:38] ================ sysctl_test (11 subtests) ================= >> [23:16:38] [PASSED] sysctl_test_api_dointvec_null_tbl_data >> [23:16:38] [PASSED] sysctl_test_api_dointvec_table_maxlen_unset >> [23:16:38] [PASSED] sysctl_test_api_dointvec_table_len_is_zero >> [23:16:38] [PASSED] sysctl_test_api_dointvec_table_read_but_position_set >> [23:16:38] [PASSED] sysctl_test_dointvec_read_happy_single_positive >> [23:16:38] [PASSED] sysctl_test_dointvec_read_happy_single_negative >> [23:16:38] [PASSED] sysctl_test_dointvec_write_happy_single_positive >> [23:16:38] [PASSED] sysctl_test_dointvec_write_happy_single_negative >> [23:16:38] [PASSED] sysctl_test_api_dointvec_write_single_less_int_min >> [23:16:38] [PASSED] sysctl_test_api_dointvec_write_single_greater_int_max >> [23:16:38] [PASSED] sysctl_test_register_sysctl_sz_invalid_extra_value >> [23:16:38] =================== [PASSED] sysctl_test =================== >> [23:16:38] ============================================================ >> [23:16:38] Testing complete. Ran 11 tests: passed: 11 >> >> Signed-off-by: Wen Yang >> Cc: Joel Granados >> Cc: Luis Chamberlain >> Cc: Kees Cook >> Cc: Eric W. Biederman >> Cc: Christian Brauner >> Cc: Dave Young >> Cc: linux-kernel@vger.kernel.org >> --- >> kernel/sysctl.c | 211 ++++++++++++++++++++---------------------------- >> 1 file changed, 86 insertions(+), 125 deletions(-) >> >> diff --git a/kernel/sysctl.c b/kernel/sysctl.c >> index 7ae7a4136855..250dc9057259 100644 >> --- a/kernel/sysctl.c >> +++ b/kernel/sysctl.c >> @@ -424,22 +424,44 @@ static void proc_put_char(void **buf, size_t *size, char c) >> } >> } >> >> -static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp, >> - int *valp, >> - int write, void *data) >> +/** >> + * struct do_proc_dointvec_minmax_conv_param - proc_dointvec_minmax() range checking structure >> + * @min: the minimum allowable value >> + * @max: the maximum allowable value >> + * >> + * The do_proc_dointvec_minmax_conv_param structure provides the >> + * minimum and maximum values for doing range checking for those sysctl >> + * parameters that use the proc_dointvec_minmax(), proc_dou8vec_minmax() and so on. >> + */ >> +struct do_proc_dointvec_minmax_conv_param { >> + int min; >> + int max; >> +}; >> + >> +static inline int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp, >> + int *valp, >> + int write, void *data) >> { >> + struct do_proc_dointvec_minmax_conv_param *param = data; >> + long min, max; >> + long val; >> + >> if (write) { >> if (*negp) { >> - if (*lvalp > (unsigned long) INT_MAX + 1) >> - return -EINVAL; >> - WRITE_ONCE(*valp, -*lvalp); >> + max = param ? param->max : 0; >> + min = param ? param->min : INT_MIN; >> + val = -*lvalp; >> } else { >> - if (*lvalp > (unsigned long) INT_MAX) >> - return -EINVAL; >> - WRITE_ONCE(*valp, *lvalp); >> + max = param ? param->max : INT_MAX; >> + min = param ? param->min : 0; >> + val = *lvalp; >> } >> + >> + if (val > max || val < min) >> + return -EINVAL; >> + WRITE_ONCE(*valp, (int)val); >> } else { >> - int val = READ_ONCE(*valp); >> + val = (int)READ_ONCE(*valp); >> if (val < 0) { >> *negp = true; >> *lvalp = -(unsigned long)val; >> @@ -448,21 +470,44 @@ static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp, >> *lvalp = (unsigned long)val; >> } >> } >> + >> return 0; >> } >> >> -static int do_proc_douintvec_conv(unsigned long *lvalp, >> - unsigned int *valp, >> - int write, void *data) >> +/** >> + * struct do_proc_douintvec_minmax_conv_param - proc_douintvec_minmax() range checking structure >> + * @min: minimum allowable value >> + * @max: maximum allowable value >> + * >> + * The do_proc_douintvec_minmax_conv_param structure provides the >> + * minimum and maximum values for doing range checking for those sysctl >> + * parameters that use the proc_douintvec_minmax() handler. >> + */ >> +struct do_proc_douintvec_minmax_conv_param { >> + unsigned int min; >> + unsigned int max; >> +}; >> + >> +static inline int do_proc_douintvec_minmax_conv(unsigned long *lvalp, >> + unsigned int *valp, >> + int write, void *data) >> { >> + struct do_proc_douintvec_minmax_conv_param *param = data; >> + unsigned long min, max; >> + >> if (write) { >> - if (*lvalp > UINT_MAX) >> + max = param ? param->max : UINT_MAX; >> + min = param ? param->min : 0; >> + >> + if (*lvalp > max || *lvalp < min) >> return -EINVAL; >> - WRITE_ONCE(*valp, *lvalp); >> + >> + WRITE_ONCE(*valp, (unsigned int)*lvalp); >> } else { >> unsigned int val = READ_ONCE(*valp); >> *lvalp = (unsigned long)val; >> } >> + >> return 0; >> } >> >> @@ -489,7 +534,7 @@ static int __do_proc_dointvec(void *tbl_data, const struct ctl_table *table, >> left = *lenp; >> >> if (!conv) >> - conv = do_proc_dointvec_conv; >> + conv = do_proc_dointvec_minmax_conv; >> >> if (write) { >> if (proc_first_pos_non_zero_ignore(ppos, table)) >> @@ -667,7 +712,7 @@ static int __do_proc_douintvec(void *tbl_data, const struct ctl_table *table, >> } >> >> if (!conv) >> - conv = do_proc_douintvec_conv; >> + conv = do_proc_douintvec_minmax_conv; >> >> if (write) >> return do_proc_douintvec_w(i, table, buffer, lenp, ppos, >> @@ -762,7 +807,7 @@ int proc_douintvec(const struct ctl_table *table, int write, void *buffer, >> size_t *lenp, loff_t *ppos) >> { >> return do_proc_douintvec(table, write, buffer, lenp, ppos, >> - do_proc_douintvec_conv, NULL); >> + do_proc_douintvec_minmax_conv, NULL); >> } >> >> /* >> @@ -808,46 +853,6 @@ static int proc_taint(const struct ctl_table *table, int write, >> return err; >> } >> >> -/** >> - * struct do_proc_dointvec_minmax_conv_param - proc_dointvec_minmax() range checking structure >> - * @min: pointer to minimum allowable value >> - * @max: pointer to maximum allowable value >> - * >> - * The do_proc_dointvec_minmax_conv_param structure provides the >> - * minimum and maximum values for doing range checking for those sysctl >> - * parameters that use the proc_dointvec_minmax() handler. >> - */ >> -struct do_proc_dointvec_minmax_conv_param { >> - int *min; >> - int *max; >> -}; >> - >> -static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp, >> - int *valp, >> - int write, void *data) >> -{ >> - int tmp, ret; >> - struct do_proc_dointvec_minmax_conv_param *param = data; >> - /* >> - * If writing, first do so via a temporary local int so we can >> - * bounds-check it before touching *valp. >> - */ >> - int *ip = write ? &tmp : valp; >> - >> - ret = do_proc_dointvec_conv(negp, lvalp, ip, write, data); >> - if (ret) >> - return ret; >> - >> - if (write) { >> - if ((param->min && *param->min > tmp) || >> - (param->max && *param->max < tmp)) >> - return -EINVAL; >> - WRITE_ONCE(*valp, tmp); >> - } >> - >> - return 0; >> -} >> - >> /** >> * proc_dointvec_minmax - read a vector of integers with min/max values >> * @table: the sysctl table >> @@ -867,53 +872,14 @@ static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp, >> int proc_dointvec_minmax(const struct ctl_table *table, int write, >> void *buffer, size_t *lenp, loff_t *ppos) >> { >> - struct do_proc_dointvec_minmax_conv_param param = { >> - .min = (int *) table->extra1, >> - .max = (int *) table->extra2, >> - }; >> + struct do_proc_dointvec_minmax_conv_param param; >> + >> + param.min = (table->extra1) ? *(int *) table->extra1 : INT_MIN; >> + param.max = (table->extra2) ? *(int *) table->extra2 : INT_MAX; >> return do_proc_dointvec(table, write, buffer, lenp, ppos, >> do_proc_dointvec_minmax_conv, ¶m); >> } >> >> -/** >> - * struct do_proc_douintvec_minmax_conv_param - proc_douintvec_minmax() range checking structure >> - * @min: pointer to minimum allowable value >> - * @max: pointer to maximum allowable value >> - * >> - * The do_proc_douintvec_minmax_conv_param structure provides the >> - * minimum and maximum values for doing range checking for those sysctl >> - * parameters that use the proc_douintvec_minmax() handler. >> - */ >> -struct do_proc_douintvec_minmax_conv_param { >> - unsigned int *min; >> - unsigned int *max; >> -}; >> - >> -static int do_proc_douintvec_minmax_conv(unsigned long *lvalp, >> - unsigned int *valp, >> - int write, void *data) >> -{ >> - int ret; >> - unsigned int tmp; >> - struct do_proc_douintvec_minmax_conv_param *param = data; >> - /* write via temporary local uint for bounds-checking */ >> - unsigned int *up = write ? &tmp : valp; >> - >> - ret = do_proc_douintvec_conv(lvalp, up, write, data); >> - if (ret) >> - return ret; >> - >> - if (write) { >> - if ((param->min && *param->min > tmp) || >> - (param->max && *param->max < tmp)) >> - return -ERANGE; >> - >> - WRITE_ONCE(*valp, tmp); >> - } >> - >> - return 0; >> -} >> - >> /** >> * proc_douintvec_minmax - read a vector of unsigned ints with min/max values >> * @table: the sysctl table >> @@ -936,10 +902,11 @@ static int do_proc_douintvec_minmax_conv(unsigned long *lvalp, >> int proc_douintvec_minmax(const struct ctl_table *table, int write, >> void *buffer, size_t *lenp, loff_t *ppos) >> { >> - struct do_proc_douintvec_minmax_conv_param param = { >> - .min = (unsigned int *) table->extra1, >> - .max = (unsigned int *) table->extra2, >> - }; >> + struct do_proc_douintvec_minmax_conv_param param; >> + >> + param.min = (table->extra1) ? *(unsigned int *) table->extra1 : 0; >> + param.max = (table->extra2) ? *(unsigned int *) table->extra2 : UINT_MAX; >> + >> return do_proc_douintvec(table, write, buffer, lenp, ppos, >> do_proc_douintvec_minmax_conv, ¶m); >> } >> @@ -965,23 +932,17 @@ int proc_dou8vec_minmax(const struct ctl_table *table, int write, >> void *buffer, size_t *lenp, loff_t *ppos) >> { >> struct ctl_table tmp; >> - unsigned int min = 0, max = 255U, val; >> + unsigned int val; >> u8 *data = table->data; >> - struct param = { >> - .min = &min, >> - .max = &max, >> - }; >> + struct do_proc_douintvec_minmax_conv_param param; >> int res; >> >> /* Do not support arrays yet. */ >> if (table->maxlen != sizeof(u8)) >> return -EINVAL; >> >> - if (table->extra1) >> - min = *(unsigned int *) table->extra1; >> - if (table->extra2) >> - max = *(unsigned int *) table->extra2; >> - >> + param.min = (table->extra1) ? *(unsigned int *) table->extra1 : 0; >> + param.max = (table->extra2) ? *(unsigned int *) table->extra2 : 255U; >> tmp = *table; >> >> tmp.maxlen = sizeof(val); >> @@ -1022,7 +983,7 @@ static int __do_proc_doulongvec_minmax(void *data, >> void *buffer, size_t *lenp, loff_t *ppos, >> unsigned long convmul, unsigned long convdiv) >> { >> - unsigned long *i, *min, *max; >> + unsigned long *i, min, max; >> int vleft, first = 1, err = 0; >> size_t left; >> char *p; >> @@ -1033,8 +994,9 @@ static int __do_proc_doulongvec_minmax(void *data, >> } >> >> i = data; >> - min = table->extra1; >> - max = table->extra2; >> + min = (table->extra1) ? *(unsigned long *) table->extra1 : 0; >> + max = (table->extra2) ? *(unsigned long *) table->extra2 : ULONG_MAX; >> + >> vleft = table->maxlen / sizeof(unsigned long); >> left = *lenp; >> >> @@ -1066,7 +1028,7 @@ static int __do_proc_doulongvec_minmax(void *data, >> } >> >> val = convmul * val / convdiv; >> - if ((min && val < *min) || (max && val > *max)) { >> + if ((val < min) || (val > max)) { >> err = -EINVAL; >> break; >> } >> @@ -1236,8 +1198,7 @@ static int do_proc_dointvec_ms_jiffies_minmax_conv(bool *negp, unsigned long *lv >> return ret; >> >> if (write) { >> - if ((param->min && *param->min > tmp) || >> - (param->max && *param->max < tmp)) >> + if ((param->min > tmp) || (param->max < tmp)) >> return -EINVAL; >> *valp = tmp; >> } >> @@ -1269,10 +1230,10 @@ int proc_dointvec_jiffies(const struct ctl_table *table, int write, >> int proc_dointvec_ms_jiffies_minmax(const struct ctl_table *table, int write, >> void *buffer, size_t *lenp, loff_t *ppos) >> { >> - struct do_proc_dointvec_minmax_conv_param param = { >> - .min = (int *) table->extra1, >> - .max = (int *) table->extra2, >> - }; >> + struct do_proc_dointvec_minmax_conv_param param; >> + >> + param.min = (table->extra1) ? *(int *) table->extra1 : INT_MIN; >> + param.max = (table->extra2) ? *(int *) table->extra2 : INT_MAX; >> return do_proc_dointvec(table, write, buffer, lenp, ppos, >> do_proc_dointvec_ms_jiffies_minmax_conv, ¶m); >> } >> -- >> 2.25.1 >> >