linux-hardening - Re: Re: Re: [PATCH v2] treewide: const qualify ctl

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: <f4lfo2fb7ajogucsvisfd5sg2avykavmkizr6ycsllcrco4mo3@qt2zx4zp57zh>
Date: Mon, 27 Jan 2025 14:49:55 +0100
From: Joel Granados <joel.granados@...nel.org>
To: Ard Biesheuvel <ardb@...nel.org>
Cc: Alexander Gordeev <agordeev@...ux.ibm.com>, 
	Thomas Weißschuh <linux@...ssschuh.net>, Kees Cook <kees@...nel.org>, 
	Luis Chamberlain <mcgrof@...nel.org>, linux-arm-kernel@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org, linux-riscv@...ts.infradead.org, 
	linux-s390@...r.kernel.org, linux-crypto@...r.kernel.org, 
	openipmi-developer@...ts.sourceforge.net, intel-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org, 
	intel-xe@...ts.freedesktop.org, linux-hyperv@...r.kernel.org, linux-rdma@...r.kernel.org, 
	linux-raid@...r.kernel.org, linux-scsi@...r.kernel.org, linux-serial@...r.kernel.org, 
	xen-devel@...ts.xenproject.org, linux-aio@...ck.org, linux-fsdevel@...r.kernel.org, 
	netfs@...ts.linux.dev, codalist@...a.cs.cmu.edu, linux-mm@...ck.org, 
	linux-nfs@...r.kernel.org, ocfs2-devel@...ts.linux.dev, fsverity@...ts.linux.dev, 
	linux-xfs@...r.kernel.org, io-uring@...r.kernel.org, bpf@...r.kernel.org, 
	kexec@...ts.infradead.org, linux-trace-kernel@...r.kernel.org, 
	linux-hardening@...r.kernel.org, apparmor@...ts.ubuntu.com, linux-security-module@...r.kernel.org, 
	keyrings@...r.kernel.org, Song Liu <song@...nel.org>, 
	"Steven Rostedt (Google)" <rostedt@...dmis.org>, "Martin K. Petersen" <martin.petersen@...cle.com>, 
	"Darrick J. Wong" <djwong@...nel.org>, Jani Nikula <jani.nikula@...el.com>, 
	Corey Minyard <cminyard@...sta.com>
Subject: Re: Re: Re: [PATCH v2] treewide: const qualify ctl_tables where
 applicable

On Wed, Jan 22, 2025 at 01:41:35PM +0100, Ard Biesheuvel wrote:
> On Wed, 22 Jan 2025 at 13:25, Joel Granados <joel.granados@...nel.org> wrote:
> >
> > On Tue, Jan 21, 2025 at 02:40:16PM +0100, Alexander Gordeev wrote:
> > > On Fri, Jan 10, 2025 at 03:16:08PM +0100, Joel Granados wrote:
> > >
> > > Hi Joel,
> > >
> > > > Add the const qualifier to all the ctl_tables in the tree except for
> > > > watchdog_hardlockup_sysctl, memory_allocation_profiling_sysctls,
> > > > loadpin_sysctl_table and the ones calling register_net_sysctl (./net,
> > > > drivers/inifiniband dirs). These are special cases as they use a
> > > > registration function with a non-const qualified ctl_table argument or
> > > > modify the arrays before passing them on to the registration function.
> > > >
> > > > Constifying ctl_table structs will prevent the modification of
> > > > proc_handler function pointers as the arrays would reside in .rodata.
> > > > This is made possible after commit 78eb4ea25cd5 ("sysctl: treewide:
> > > > constify the ctl_table argument of proc_handlers") constified all the
> > > > proc_handlers.
> > >
> > > I could identify at least these occurences in s390 code as well:
> > Hey Alexander
> >
> > Thx for bringing these to my attention. I had completely missed them as
> > the spatch only deals with ctl_tables outside functions.
> >
> > Short answer:
> > These should not be included in the current patch because they are a
> > different pattern from how sysctl tables are usually used. So I will not
> > include them.
> >
> > With that said, I think it might be interesting to look closer at them
> > as they seem to be complicating the proc_handler (I have to look at them
> > closer).
> >
> > I see that they are defining a ctl_table struct within the functions and
> > just using the data (from the incoming ctl_table) to forward things down
> > to proc_do{u,}intvec_* functions. This is very odd and I have only seen
> > it done in order to change the incoming ctl_table (which is not what is
> > being done here).
> >
> > I will take a closer look after the merge window and circle back with
> > more info. Might take me a while as I'm not very familiar with s390
> > code; any additional information on why those are being used inside the
> > functions would be helpfull.
> >
> 
> Using const data on the stack is not as useful, because the stack is
> always mapped writable.
> 
> Global data structures marked 'const' will be moved into an ELF
> section that is typically mapped read-only in its entirely, and so the
> data cannot be modified by writing to it directly. No such protection
> is possible for the stack, and so the constness there is only enforced
> at compile time.
I completely agree with you. No reason to use const within those
functions. But why define those ctl_tables in function to begin with.
Can't you just use the ones that are defined outside the functions?

Best


-- 

Joel Granados