linux-kernel - Re: Re: [PATCH v2 2/3] sysctl: Fix underflow value setting risk in vm

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] [day] [month] [year] [list]

Message-ID: <gqmqk4wqbyvktq3qfjte6dmo5pb2ydeulqzwsxc6zat7scj5md@er2gwfgcmw2d>
Date: Wed, 18 Dec 2024 14:32:34 +0100
From: Joel Granados <joel.granados@...nel.org>
To: Nicolas Bouchinet <nicolas.bouchinet@...p-os.org>
Cc: linux-kernel@...r.kernel.org, linux-serial@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org, Nicolas Bouchinet <nicolas.bouchinet@....gouv.fr>, 
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Jiri Slaby <jirislaby@...nel.org>, 
	Alexander Viro <viro@...iv.linux.org.uk>, Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>, 
	Luis Chamberlain <mcgrof@...nel.org>, Kees Cook <kees@...nel.org>, 
	Joel Granados <j.granados@...sung.com>, Andrew Morton <akpm@...ux-foundation.org>, 
	Neil Horman <nhorman@...driver.com>, Lin Feng <linf@...gsu.com>, Theodore Ts'o <tytso@....edu>
Subject: Re: Re: [PATCH v2 2/3] sysctl: Fix underflow value setting risk in
 vm_table

On Tue, Dec 17, 2024 at 02:57:51PM +0100, Nicolas Bouchinet wrote:
> Hi Joel,
> 
> I've pushed patchset version 3 :
> https://lore.kernel.org/all/20241217132908.38096-1-nicolas.bouchinet@clip-os.org/.
> 
> On 11/20/24 13:53, Joel Granados wrote:
> > On Thu, Nov 14, 2024 at 05:25:51PM +0100, nicolas.bouchinet@...p-os.org wrote:
> >> From: Nicolas Bouchinet <nicolas.bouchinet@....gouv.fr>
> >>
> >> Commit 3b3376f222e3 ("sysctl.c: fix underflow value setting risk in
> >> vm_table") fixes underflow value setting risk in vm_table but misses
> >> vdso_enabled sysctl.
> >>
> >> vdso_enabled sysctl is initialized with .extra1 value as SYSCTL_ZERO to
> >> avoid negative value writes but the proc_handler is proc_dointvec and not
> >> proc_dointvec_minmax and thus do not uses .extra1 and .extra2.
> >>
> >> The following command thus works :
> >>
> >> `# echo -1 > /proc/sys/vm/vdso_enabled`
> > It would be interesting to know what happens when you do a
> > # echo (INT_MAX + 1) > /proc/sys/vm/vdso_enabled
> >
> > This is the reasons why I'm interested in such a test:
> >
> > 1. Both proc_dointvec and proc_dointvec_minmax (calls proc_dointvec) have a
> >     overflow check where they will return -EINVAL if what is given by the user is
> >     greater than (unsiged long)INT_MAX; this will evaluate can evaluate to true
> >     or false depending on the architecture where we are running.
> >
> > 2. I noticed that vdso_enabled is an unsigned long. And so the expectation is
> >     that the range is 0 to ULONG_MAX, which in some cases (depending on the arch)
> >     would not be the case.
>  From my observations, vdso_enabled is a unsigned int. If one wants to
> convert to an unsigned long, proc_doulongvec_minmax should be used
> instead.
Yep, 100% agree, I miss-read and commented incorrectly. Just ignore my
previous comment; I don't know what I was smoking...

> 
> IMHO, the main issues are that .data variable type can differ from the
> return type of .proc_handler function. This can lead to undefined
> behaviors and eventually vulnerabilities.
I totally agree that it can lead to unexpected behavior. Would have to
look at a specific case to see if it is really "undefined". 

> 
> .extra1 and .extra2 can also be used with proc_handlers that do not
> uses them.
In this case they are just silently ignored. Leading the developer to
believe that they are range checked, when they are really not.

> I think sysctl_check_table() could be enhanced to control
> this behavior.
This might be the case. I can review a proposal if you send it out.

Best

-- 

Joel Granados