| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8af383e9-d90d-492d-81e5-91d8edfe1508@linux.dev>
Date: Sat, 21 Sep 2024 16:19:01 +0800
From: Wen Yang <wen.yang@...ux.dev>
To: Thomas Weißschuh <thomas@...ch.de>
Cc: "Eric W . Biederman" <ebiederm@...ssion.com>,
Luis Chamberlain <mcgrof@...nel.org>, Kees Cook <keescook@...omium.org>,
Joel Granados <j.granados@...sung.com>,
Christian Brauner <brauner@...nel.org>, linux-kernel@...r.kernel.org,
Dave Young <dyoung@...hat.com>
Subject: Re: [PATCH v3 2/5] sysctl: support encoding values directly in the
table entry
On 2024/9/15 14:25, Thomas Weißschuh wrote:
> On 2024-09-15 10:08:28+0000, Wen Yang wrote:
>> Eric points out: "by turning .extra1 and .extra2 into longs instead of
>> keeping them as pointers and needing constants to be pointed at somewhere
>> .. The only people I can see who find a significant benefit by
>> consolidating all of the constants into one place are people who know how
>> to stomp kernel memory."
>>
>> This patch supports encoding values directly in table entries through the
>> following work:
>> - extra1/extra2 and min/max are placed in one union to ensure that the
>> previous code is not broken, then we have time to gradually remove these
>> unnecessary extra1/extra2;
>
> The union shouldn't be necessary for backward compatibility.
> It could just as well be a struct.
> The union however saves some space.
>
>> - two bits were used to represent the information of the above union:
>> SYSCTL_FLAG_MIN: 0, using extra1. 1, using min.
>> SYSCTL_FLAG_MAX: 0, using extra2. 1, using max.
>> - since the proc file's mode field only uses 9 bits(777), we could use the
>> additional two bits(S_ISUID and S_ISGID) to temporarily represent
>> SYSCTL_FLAG_MIN and SYSCTL_FLAG_MAX.
>> - added some helper macros.
>>
>> By introducing long min/max to replace void * extra1/extra2 in most cases,
>> unnecessary variables can be removed to save memory and avoid attacks.
>>
>> Signed-off-by: Wen Yang <wen.yang@...ux.dev>
>> Cc: Luis Chamberlain <mcgrof@...nel.org>
>> Cc: Kees Cook <keescook@...omium.org>
>> Cc: Joel Granados <j.granados@...sung.com>
>> Cc: Eric W. Biederman <ebiederm@...ssion.com>
>> Cc: Christian Brauner <brauner@...nel.org>
>> Cc: Dave Young <dyoung@...hat.com>
>> Cc: linux-kernel@...r.kernel.org
>> ---
>> fs/proc/proc_sysctl.c | 8 +++--
>> include/linux/sysctl.h | 71 ++++++++++++++++++++++++++++++++++++------
>> 2 files changed, 67 insertions(+), 12 deletions(-)
>>
>> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
>> index 90c99eb1abf6..e88d1dca2a80 100644
>> --- a/fs/proc/proc_sysctl.c
>> +++ b/fs/proc/proc_sysctl.c
>> @@ -848,8 +848,11 @@ static int proc_sys_getattr(struct mnt_idmap *idmap,
>> return PTR_ERR(head);
>>
>> generic_fillattr(&nop_mnt_idmap, request_mask, inode, stat);
>> - if (table)
>> + if (table) {
>> stat->mode = (stat->mode & S_IFMT) | table->mode;
>> + stat->mode &= ~SYSCTL_FLAG_MIN;
>> + stat->mode &= ~SYSCTL_FLAG_MAX;
>> + }
>>
>> sysctl_head_finish(head);
>> return 0;
>> @@ -1163,7 +1166,8 @@ static int sysctl_check_table(const char *path, struct ctl_table_header *header)
>> if (!entry->proc_handler)
>> err |= sysctl_err(path, entry, "No proc_handler");
>>
>> - if ((entry->mode & (S_IRUGO|S_IWUGO)) != entry->mode)
>> + if ((entry->mode & (S_IRUGO|S_IWUGO|SYSCTL_FLAG_MIN|SYSCTL_FLAG_MAX))
>> + != entry->mode)
>> err |= sysctl_err(path, entry, "bogus .mode 0%o",
>> entry->mode);
>> }
>> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
>> index 20e3914ec53f..8e27e8350ca8 100644
>> --- a/include/linux/sysctl.h
>> +++ b/include/linux/sysctl.h
>> @@ -28,6 +28,7 @@
>> #include <linux/rbtree.h>
>> #include <linux/uidgid.h>
>> #include <uapi/linux/sysctl.h>
>> +#include <uapi/linux/stat.h>
>>
>> /* For the /proc/sys support */
>> struct completion;
>> @@ -61,6 +62,37 @@ extern const int sysctl_vals[];
>>
>> extern const unsigned long sysctl_long_vals[];
>>
>> +#define SYSCTL_NUM_ZERO (0L)
>> +#define SYSCTL_NUM_ONE (1L)
>> +#define SYSCTL_NUM_TWO (2L)
>> +#define SYSCTL_NUM_THREE (3L)
>> +#define SYSCTL_NUM_FOUR (4L)
>> +#define SYSCTL_NUM_FIVE (5L)
>> +#define SYSCTL_NUM_SIX (6L)
>> +#define SYSCTL_NUM_SEVEN (7L)
>> +#define SYSCTL_NUM_EIGHT (8L)
>> +#define SYSCTL_NUM_NINE (9L)
>> +#define SYSCTL_NUM_TEN (10L)
>> +#define SYSCTL_NUM_SIXTEEN (16L)
>> +#define SYSCTL_NUM_THIRTY_ONE (31L)
>> +#define SYSCTL_NUM_NEG_THIRTY_ONE (-31L)
>> +#define SYSCTL_NUM_ONE_HUNDRED (100L)
>> +#define SYSCTL_NUM_TWO_HUNDRED (200L)
>> +#define SYSCTL_NUM_S8_MAX (127L)
>> +#define SYSCTL_NUM_U8_MAX (255L)
>> +#define SYSCTL_NUM_FIVE_HUNDRED (500L)
>> +#define SYSCTL_NUM_ONE_THOUSAND (1000L)
>> +#define SYSCTL_NUM_THREE_THOUSAND (3000L)
>> +#define SYSCTL_NUM_16K (16 * 1024L)
>> +#define SYSCTL_NUM_16M (16 * 1024 * 1024L)
>> +#define SYSCTL_NUM_SEC_PER_HOUR (60 * 60L)
>> +#define SYSCTL_NUM_U16_MAX (65535L)
>> +#define SYSCTL_NUM_SEC_PER_DAY (24 * 60 * 60L)
>> +#define SYSCTL_NUM_MS_PER_DAY (24 * 60 * 60 * 1000L)
>> +#define SYSCTL_NUM_INT_MAX (INT_MAX)
>> +#define SYSCTL_NUM_NEG_ONE (-1)
>> +#define SYSCTL_NUM_LONG_MAX (LONG_MAX)
>
> Are all those constants really needed?
> In the past they were useful as they referenced the array of constants.
> But now people can just use the number directly, or even better a
> subsystem-specific constant.
>
Okay, we will make the modifications according to your suggestions.
>> +
>> typedef int proc_handler(const struct ctl_table *ctl, int write, void *buffer,
>> size_t *lenp, loff_t *ppos);
>>
>> @@ -131,6 +163,9 @@ static inline void *proc_sys_poll_event(struct ctl_table_poll *poll)
>> #define DEFINE_CTL_TABLE_POLL(name) \
>> struct ctl_table_poll name = __CTL_TABLE_POLL_INITIALIZER(name)
>>
>> +#define SYSCTL_FLAG_MIN S_ISUID
>> +#define SYSCTL_FLAG_MAX S_ISGID
>
> Is it ever useful to have only one of these flags?
> IMO this could be a single flag.
>
There are a few edge cases that require two flags, such as the
following code snippet:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/net/sctp/sysctl.c#n103
[SCTP_RTO_MIN_IDX] = {
.procname = "rto_min",
.data = &init_net.sctp.rto_min,
...
.extra1 = SYSCTL_ONE,
.extra2 = &init_net.sctp.rto_max
},
extra1 points to a constant unsigned integer, but extra2 points to a
field in the struct sctp.association structure
>> +
>> /* A sysctl table is an array of struct ctl_table: */
>> struct ctl_table {
>> const char *procname; /* Text ID for /proc/sys, or zero */
>> @@ -139,8 +174,16 @@ struct ctl_table {
>> umode_t mode;
>> proc_handler *proc_handler; /* Callback for text formatting */
>> struct ctl_table_poll *poll;
>> - void *extra1;
>> - void *extra2;
>> + union {
>> + struct {
>> + void *extra1;
>> + void *extra2;
>> + };
>> + struct {
>> + long min;
>> + long max;
>> + };
>> + };
>> } __randomize_layout;
>>
>> struct ctl_node {
>> @@ -214,42 +257,50 @@ struct ctl_table_root {
>>
>> static inline unsigned int sysctl_range_min_u8(const struct ctl_table *table)
>> {
>> - return (table->extra1) ? *(unsigned int *) table->extra1 : 0;
>> + return (table->mode & SYSCTL_FLAG_MIN) ? table->min :
>> + (table->extra1) ? *(unsigned int *) table->extra1 : 0;
>
> All these repetitive functions could probably be replaced by one macro.
> Keeping the users more direct and avoiding all the repetition.
>
Okay, we will make the modifications according to your suggestions and
send v4 soon.
--
Best wishes,
Wen
Powered by blists - more mailing lists