netdev - Re: [PATCH v3 00/11] sysctl: treewide: constify 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: <d11f875e-4fb5-46dd-a412-84818208c575@t-8ch.de>
Date: Thu, 25 Apr 2024 22:34:52 +0200
From: Thomas Weißschuh <linux@...ssschuh.net>
To: Joel Granados <j.granados@...sung.com>
Cc: Jakub Kicinski <kuba@...nel.org>, Luis Chamberlain <mcgrof@...nel.org>, 
	Kees Cook <keescook@...omium.org>, Eric Dumazet <edumazet@...gle.com>, 
	Dave Chinner <david@...morbit.com>, linux-fsdevel@...r.kernel.org, netdev@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-s390@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-riscv@...ts.infradead.org, linux-mm@...ck.org, linux-security-module@...r.kernel.org, 
	bpf@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org, linux-xfs@...r.kernel.org, 
	linux-trace-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org, 
	netfilter-devel@...r.kernel.org, coreteam@...filter.org, kexec@...ts.infradead.org, 
	linux-hardening@...r.kernel.org, bridge@...ts.linux.dev, lvs-devel@...r.kernel.org, 
	linux-rdma@...r.kernel.org, rds-devel@....oracle.com, linux-sctp@...r.kernel.org, 
	linux-nfs@...r.kernel.org, apparmor@...ts.ubuntu.com
Subject: Re: [PATCH v3 00/11] sysctl: treewide: constify ctl_table argument
 of sysctl handlers

Hi Joel,

On 2024-04-25 13:04:12+0000, Joel Granados wrote:
> On Wed, Apr 24, 2024 at 08:12:34PM -0700, Jakub Kicinski wrote:
> > On Tue, 23 Apr 2024 09:54:35 +0200 Thomas Weißschuh wrote:
> > > The series was split from my larger series sysctl-const series [0].
> > > It only focusses on the proc_handlers but is an important step to be
> > > able to move all static definitions of ctl_table into .rodata.
> > 
> > Split this per subsystem, please.
> It is tricky to do that because it changes the first argument (ctl*) to
> const in the proc_handler function type defined in sysclt.h:
> "
> -typedef int proc_handler(struct ctl_table *ctl, int write, void *buffer,
> +typedef int proc_handler(const struct ctl_table *ctl, int write, void *buffer,
>                 size_t *lenp, loff_t *ppos);
> "
> This means that all the proc_handlers need to change at the same time.
> 
> However, there is an alternative way to do this that allows chunking. We
> first define the proc_handler as a void pointer (casting it where it is
> being used) [1]. Then we could do the constification by subsystem (like
> Jakub proposes). Finally we can "revert the void pointer change so we
> don't have one size fit all pointer as our proc_handler [2].
> 
> Here are some comments about the alternative:
> 1. We would need to make the first argument const in all the derived
>    proc_handlers [3] 
> 2. There would be no undefined behavior for two reasons:
>    2.1. There is no case where we change the first argument. We know
>         this because there are no compile errors after we make it const.
>    2.2. We would always go from non-const to const. This is the case
>         because all the stuff that is unchanged in non-const.
> 3. If the idea sticks, it should go into mainline as one patchset. I
>    would not like to have a void* proc_handler in a kernel release.
> 4. I think this is a "win/win" solution were the constification goes
>    through and it is divided in such a way that it is reviewable.
> 
> I would really like to hear what ppl think about this "heretic"
> alternative. @Thomas, @Luis, @Kees @Jakub?

Thanks for that alternative, I'm not a big fan though.

Besides the wonky syntax, Control Flow Integrity should trap on
this construct. Functions are called through different pointers than
their actual types which is exactly what CFI is meant to prevent.

Maybe people find it easier to review when using
"--word-diff" and/or "-U0" with git diff/show.
There is really nothing going an besides adding a few "const"s.

But if the consensus prefers this solution, I'll be happy to adopt it.

> [1] https://git.kernel.org/pub/scm/linux/kernel/git/joel.granados/linux.git/commit/?h=jag/constfy_treewide_alternative&id=4a383503b1ea650d4e12c1f5838974e879f5aa6f
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/joel.granados/linux.git/commit/?h=jag/constfy_treewide_alternative&id=a3be65973d27ec2933b9e81e1bec60be3a9b460d
> [3] proc_dostring, proc_dobool, proc_dointvec....


Thomas