linux-kernel - Re: Re: [PATCH v5] sysctl: simplify the min/max boundary check

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ov6x26vw4rq5ekz4fy2t23xbtkh2dkkrfrkzp7dvkhy2djm4vl@2b7batukhrbm>
Date: Mon, 27 Jan 2025 14:34:12 +0100
From: Joel Granados <joel.granados@...nel.org>
To: "Eric W. Biederman" <ebiederm@...ssion.com>
Cc: Wen Yang <wen.yang@...ux.dev>, Luis Chamberlain <mcgrof@...nel.org>, 
	Kees Cook <keescook@...omium.org>, Christian Brauner <brauner@...nel.org>, 
	Dave Young <dyoung@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: Re: [PATCH v5] sysctl: simplify the min/max boundary check

On Thu, Jan 23, 2025 at 12:30:25PM -0600, Eric W. Biederman wrote:
> "Eric W. Biederman" <ebiederm@...ssion.com> writes:
> 
> > Joel Granados <joel.granados@...nel.org> writes:
> >
> >> On Sun, Jan 19, 2025 at 10:59:21PM +0800, Wen Yang wrote:
> >>> 
> >>> 
> >>> 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.
...
> >> (if any). And this is why:
> >> 1. The long and the void* are most likely (depending on arch?) the same
> >>    size.
> >> 2. In [1] it is mentioned that, we would still need an extra (void*) to
> >>    address the sysctl tables that are *NOT* using extra{1,2} as min max.
> >>    This means that we need a bigger ctl_table (long extra1, long extra2
> >>    and void* extra). We will need *more* memory?
> >>
> >> I would like to be proven wrong. So this is my proposal: Instead of
> >> trying to do an incremental change, I suggest you remove the sysctl_vals
> >> shared const array and measure how much memory you actually save. You
> >> can use the ./scripts/bloat-o-meter in the linux kernel source and
> >> follow something similar to what we did in [2] to measure how much
> >> memory we are actually talking about.
> >>
> >> Once you get a hard number, then we can move forward on the memory
> >> saving front.

Hey Eric.

Thx for the clarification. Much appreciated.
> >
> > When I originally suggested this my motivation had nothing to do with memory
That makes a *lot* of sense :).

> > The sysctl_vals memory array is type unsafe and has actively
Here I understand that they are unsafe because of Integer promotion
issues exacerbated by the void* variables extra{1,2}. Please correct me
If I missed the point.

There is also the fact that you can just do a `extra1 = &sysctl_vals[15]`
and the compiler will not bark at you. At least It let me do that on my
side.

> > lead to real world bugs. AKA longs and int confusion.  One example is
> > that SYSCTL_ZERO does not properly work as a minimum to
> > proc_do_ulongvec_minmax.
That is a great example.

> >
> > Frankly those SYSCTL_XXX macros that use sysctl_vals are just plain
> > scary to work with.
I share your feeling :)

> >
> > So I suggested please making everything simpler by putting unsigned long
> > min and max in to struct ctl_table and then getting rid of extra1 and
> > extra2.  As extra1 and extra2 are almost exclusively used to implement
> > min and max.
Explicitly specifying the type will help reduce the "unsefeness" but
with all the ways that there are of using these pointers, I think we
need to think bigger and maybe try to find a more typesafe way to
represent all the interactions.

It has always struck me as strange the arbitrariness of having 2 extra
pointers. Why not just one? At the end it is a pointer and can point to
a struct that holds min, max... I do not have the answer yet, but I
think what you propose here is part of a bigger refactoring needed in
ctl_table structure. Would like to hear your thought on it if you have
any.

> >
> > The size would not change but the chance of making a hard to find and
> > understand mistake would go down dramatically.
> 
> Oh.  I forgot to add it was also about the usability of the SYSCTL_XXX
> macros.
> 
> As it is now whenever there is a new interesting value you have to add
> another SYSCTL_XXX macro, and place your value in sysctl_vals.
> 
> Compared to just having a min or a max value in struct ctl_table it
> is much more work.
> 
> Where size comes into my original suggestion is that because extra1 and
> extra2 can and probably should go away, adding min and max members to
> struct ctl_table can be done at no cost in size.
> 
> Eric
> 

Best regards

-- 

Joel Granados