Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Date:   Thu, 7 Dec 2023 13:05:57 +0100
From:   Joel Granados <j.granados@samsung.com>
To:     Luis Chamberlain <mcgrof@kernel.org>
CC:     Thomas =?utf-8?Q?Wei=C3=9Fschuh?= <linux@weissschuh.net>,
        Kees Cook <keescook@chromium.org>,
        "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        Iurii Zaikin <yzaikin@google.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        <linux-hardening@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH v2 13/18] sysctl: move sysctl type to ctl_table_header
Message-ID: <20231207120557.2om7knogbsu3cgrf@localhost>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg="pgp-sha512";
        protocol="application/pgp-signature"; boundary="db4qfr2oz7c2s6rl"
Content-Disposition: inline
In-Reply-To: <CAB=NE6UCP05MgHF85TK+t2yvbOoaW_8Yu6QEyaYMdJcGayVjFQ@mail.gmail.com>
CMS-TYPE: 201P
References: <20231204-const-sysctl-v2-0-7a5060b11447@weissschuh.net>
        <20231204-const-sysctl-v2-13-7a5060b11447@weissschuh.net>
        <ZW+lQqOSYFfeh8z2@bombadil.infradead.org>
        <4a93cdb4-031c-4f77-8697-ce7fb42afa4a@t-8ch.de>
        <CGME20231205225020eucas1p2873217afd908334ba2669aad02b95cbb@eucas1p2.samsung.com>
        <CAB=NE6UCP05MgHF85TK+t2yvbOoaW_8Yu6QEyaYMdJcGayVjFQ@mail.gmail.com>
Precedence: bulk

--db4qfr2oz7c2s6rl
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Tue, Dec 05, 2023 at 02:50:01PM -0800, Luis Chamberlain wrote:
> On Tue, Dec 5, 2023 at 2:41=E2=80=AFPM Thomas Wei=C3=9Fschuh <linux@weiss=
schuh.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=C3=9Fschuh wrote:
> > > > @@ -231,7 +231,8 @@ static int insert_header(struct ctl_dir *dir, s=
truct 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 =3D=3D 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.
>=20
> It is worse in that it is not obvious:
>=20
> +       if (table =3D=3D 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?
>=20
> > Which kind of unsafety do you envision here?
>=20
> 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.
>=20
>   Luis

--=20

Joel Granados

--db4qfr2oz7c2s6rl
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQGzBAABCgAdFiEErkcJVyXmMSXOyyeQupfNUreWQU8FAmVxtSQACgkQupfNUreW
QU+dkgv+IVdWMcibpYRbbXQoIvmp+lnMk4qkm9BsJi0/8ejv4NT5T9fFzk8Y4dYA
5hWIk9KWoG4qSBgQKOhaOv79mDsScZr4rv0PsEAYbK/ZKppmAKW3Pm2Ex6OBNyv2
BneQ8Coci9h1sWUQkoiX7+NgCQczzi6TgRpRtucbFoN+BIkYklcrwyZszBkiX6jW
dt50vOI1NGKqpu6GSZ2F22d+V58BBS7QmOmK1LRlTmPut/t5dJOfN/5d9JNNxIyJ
O8aSQ0iDXCH4E6qiZV5qC+aJBldu8T0gynVXDg/IoPInwr8d10kD5JEjxbgXo/Ey
s/aFB0qBToVQqK8xAG7T6p99QB1uIyQfBC/d9YRDhGTYiAXxYrfyc0cIFmQJ9AES
s9hh8ZVw0OLcg6lRycWfYv8eiqGYVKf5XOMsxHW+d1k5X9/L+Oxn9KCVc7bU2kez
hgqI3A3mvjW9ppDOq7CFsTH82CVZAHxNGXWBEETxCOu3q6OAGJXReXAqQEzna61A
Q1NQcRWM
=ca10
-----END PGP SIGNATURE-----

--db4qfr2oz7c2s6rl--