linux-hardening - Re: [PATCH v2 13/18] sysctl: move sysctl type to ctl_table

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: <20231207120557.2om7knogbsu3cgrf@localhost>
Date: Thu, 7 Dec 2023 13:05:57 +0100
From: Joel Granados <j.granados@...sung.com>
To: Luis Chamberlain <mcgrof@...nel.org>
CC: Thomas Weißschuh <linux@...ssschuh.net>, Kees Cook
	<keescook@...omium.org>, "Gustavo A. R. Silva" <gustavoars@...nel.org>,
	Iurii Zaikin <yzaikin@...gle.com>, Greg Kroah-Hartman
	<gregkh@...uxfoundation.org>, <linux-hardening@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH v2 13/18] sysctl: move sysctl type to ctl_table_header

On Tue, Dec 05, 2023 at 02:50:01PM -0800, Luis Chamberlain wrote:
> On Tue, Dec 5, 2023 at 2:41 PM Thomas Weißschuh <linux@...ssschuh.net> wrote:
> >
> > On 2023-12-05 14:33:38-0800, Luis Chamberlain wrote:
> > > On Mon, Dec 04, 2023 at 08:52:26AM +0100, Thomas Weißschuh wrote:
> > > > @@ -231,7 +231,8 @@ static int insert_header(struct ctl_dir *dir, struct ctl_table_header *header)
> > > >             return -EROFS;
> > > >
> > > >     /* Am I creating a permanently empty directory? */
> > > > -   if (sysctl_is_perm_empty_ctl_header(header)) {
> > > > +   if (header->ctl_table == sysctl_mount_point ||
> > > > +       sysctl_is_perm_empty_ctl_header(header)) {
> > > >             if (!RB_EMPTY_ROOT(&dir->root))
> > > >                     return -EINVAL;
> > > >             sysctl_set_perm_empty_ctl_header(dir_h);
> > >
> > > While you're at it.
> >
> > This hunk is completely gone in v3/the code that you merged.
> 
> It is worse in that it is not obvious:
> 
> +       if (table == sysctl_mount_point)
> +               sysctl_set_perm_empty_ctl_header(head);
Notice that the test is done on the header and the set is done on the
dir_h.

I mention this because here you wrote:
"sysctl_set_perm_empty_ctl_header(head)"
instead of
"sysctl_set_perm_empty_ctl_header(dir_h)"

dir_h and head are different.

Was this your concern? or did I miss your point?
> 
> > Which kind of unsafety do you envision here?
> 
> Making the code obvious during patch review hy this is needed /
> special, and if we special case this, why not remove enum, and make it
> specific to only that one table. The catch is that it is not
> immediately obvious that we actually call
> sysctl_set_perm_empty_ctl_header() in other places, and it begs the
> question if this can be cleaned up somehow.
> 
>   Luis

-- 

Joel Granados

Download attachment "signature.asc" of type "application/pgp-signature" (660 bytes)