Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
Date:   Mon, 27 Nov 2023 11:13:23 +0100
From:   Joel Granados <j.granados@samsung.com>
To:     Thomas =?utf-8?Q?Wei=C3=9Fschuh?= <linux@weissschuh.net>
CC:     Kees Cook <keescook@chromium.org>,
        "Gustavo A. R. Silva" <gustavoars@kernel.org>,
        Luis Chamberlain <mcgrof@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 RFC 0/7] sysctl: constify sysctl ctl_tables
Message-ID: <20231127101323.sdnibmf7c3d5ovye@localhost>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg="pgp-sha512";
        protocol="application/pgp-signature"; boundary="zn4hpd3vbxzf3eak"
Content-Disposition: inline
In-Reply-To: <20231125-const-sysctl-v1-0-5e881b0e0290@weissschuh.net>
CMS-TYPE: 201P
References: <CGME20231125125305eucas1p2ebdf870dd8ef46ea9d346f727b832439@eucas1p2.samsung.com>
        <20231125-const-sysctl-v1-0-5e881b0e0290@weissschuh.net>
Precedence: bulk

--zn4hpd3vbxzf3eak
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hey Thomas

In general I would like to see more clarity with the motivation and I
would also expect some system testing. My comments inline:

On Sat, Nov 25, 2023 at 01:52:49PM +0100, Thomas Wei=DFschuh wrote:
> Problem description:
>=20
> The kernel contains a lot of struct ctl_table throught the tree.
> These are very often 'static' definitions.
> It would be good to mark these tables const to avoid accidental or
> malicious modifications.
It is unclear to me what you mean here with accidental or malicious
modifications. Do you have a specific attack vector in mind? Do you
have an example of how this could happen maliciously? With
accidental, do you mean in proc/sysctl.c? Can you expand more on the
accidental part?

What happens with the code that modifies these outside the sysctl core?
Like for example in sysctl_route_net_init where the table is modified
depending on the net->user_ns? Would these non-const ctl_table pointers
be ok? would they be handled differently?

> Unfortunately the tables can not be made const because the core
> registration functions expect mutable tables.
>=20
> This is for two reasons:
>=20
> 1) sysctl_{set,clear}_perm_empty_ctl_header in the sysctl core modify
>    the table. This should be fixable by only modifying the header
>    instead of the table itself.
> 2) The table is passed to the handler function as a non-const pointer.
>=20
> This series is an aproach on fixing reason 2).
So number 2 will be sent in another set?

>=20
> Full process:
>=20
> * Introduce field proc_handler_new for const handlers (this series)
> * Migrate all core handlers to proc_handler_new (this series, partial)
>   This can hopefully be done in a big switch, as it only involves
>   functions and structures owned by the core sysctl code.
> * Migrate all other sysctl handlers to proc_handler_new.
> * Drop the old proc_handler_field.
> * Fix the sysctl core to not modify the tables anymore.
> * Adapt public sysctl APIs to take "const struct ctl_table *".
> * Teach checkpatch.pl to warn on non-const "struct ctl_table"
>   definitions.
> * Migrate definitions of "struct ctl_table" to "const" where applicable.
> =20
>=20
> Notes:
>=20
> Just casting the function pointers around would trigger
> CFI (control flow integrity) warnings.
>=20
> The name of the new handler "proc_handler_new" is a bit too long messing
> up the alignment of the table definitions.
> Maybe "proc_handler2" or "proc_handler_c" for (const) would be better.
indeed the name does not say much. "_new" looses its meaning quite fast
:)

In my experience these tree wide modifications are quite tricky. Have you
run any tests to see that everything is as it was? sysctl selftests and
0-day come to mind.

Best
>=20
> ---
> Thomas Wei=DFschuh (7):
>       sysctl: add helper sysctl_run_handler
>       bpf: cgroup: call proc handler through helper
>       sysctl: add proc_handler_new to struct ctl_table
>       net: sysctl: add new sysctl table handler to debug message
>       treewide: sysctl: migrate proc_dostring to proc_handler_new
>       treewide: sysctl: migrate proc_dobool to proc_handler_new
>       treewide: sysctl: migrate proc_dointvec to proc_handler_new
>=20
>  arch/arm/kernel/isa.c                   |  6 +--
>  arch/csky/abiv1/alignment.c             |  8 ++--
>  arch/powerpc/kernel/idle.c              |  2 +-
>  arch/riscv/kernel/vector.c              |  2 +-
>  arch/s390/kernel/debug.c                |  2 +-
>  crypto/fips.c                           |  6 +--
>  drivers/char/hpet.c                     |  2 +-
>  drivers/char/random.c                   |  4 +-
>  drivers/infiniband/core/iwcm.c          |  2 +-
>  drivers/infiniband/core/ucma.c          |  2 +-
>  drivers/macintosh/mac_hid.c             |  4 +-
>  drivers/md/md.c                         |  4 +-
>  drivers/scsi/sg.c                       |  2 +-
>  drivers/tty/tty_io.c                    |  4 +-
>  fs/coda/sysctl.c                        |  6 +--
>  fs/coredump.c                           |  6 +--
>  fs/devpts/inode.c                       |  2 +-
>  fs/lockd/svc.c                          |  4 +-
>  fs/locks.c                              |  4 +-
>  fs/nfs/nfs4sysctl.c                     |  2 +-
>  fs/nfs/sysctl.c                         |  2 +-
>  fs/notify/dnotify/dnotify.c             |  2 +-
>  fs/ntfs/sysctl.c                        |  2 +-
>  fs/ocfs2/stackglue.c                    |  2 +-
>  fs/proc/proc_sysctl.c                   | 16 ++++---
>  fs/quota/dquot.c                        |  2 +-
>  include/linux/sysctl.h                  | 29 +++++++++---
>  init/do_mounts_initrd.c                 |  2 +-
>  io_uring/io_uring.c                     |  2 +-
>  ipc/mq_sysctl.c                         |  2 +-
>  kernel/acct.c                           |  2 +-
>  kernel/bpf/cgroup.c                     |  2 +-
>  kernel/locking/lockdep.c                |  4 +-
>  kernel/printk/sysctl.c                  |  4 +-
>  kernel/reboot.c                         |  4 +-
>  kernel/seccomp.c                        |  2 +-
>  kernel/signal.c                         |  2 +-
>  kernel/sysctl-test.c                    | 20 ++++-----
>  kernel/sysctl.c                         | 80 ++++++++++++++++-----------=
------
>  lib/test_sysctl.c                       | 10 ++---
>  mm/hugetlb.c                            |  2 +-
>  mm/hugetlb_vmemmap.c                    |  2 +-
>  mm/oom_kill.c                           |  4 +-
>  net/appletalk/sysctl_net_atalk.c        |  2 +-
>  net/core/sysctl_net_core.c              | 12 ++---
>  net/ipv4/route.c                        | 18 ++++----
>  net/ipv4/sysctl_net_ipv4.c              | 38 ++++++++--------
>  net/ipv4/xfrm4_policy.c                 |  2 +-
>  net/ipv6/addrconf.c                     | 72 ++++++++++++++-------------=
--
>  net/ipv6/route.c                        |  8 ++--
>  net/ipv6/sysctl_net_ipv6.c              | 18 ++++----
>  net/ipv6/xfrm6_policy.c                 |  2 +-
>  net/mptcp/ctrl.c                        |  2 +-
>  net/netfilter/ipvs/ip_vs_ctl.c          | 36 +++++++--------
>  net/netfilter/nf_conntrack_standalone.c |  8 ++--
>  net/netfilter/nf_log.c                  |  2 +-
>  net/rds/ib_sysctl.c                     |  2 +-
>  net/rds/sysctl.c                        |  6 +--
>  net/sctp/sysctl.c                       | 26 +++++------
>  net/sunrpc/xprtrdma/transport.c         |  2 +-
>  net/sysctl_net.c                        |  5 ++-
>  net/unix/sysctl_net_unix.c              |  2 +-
>  net/x25/sysctl_net_x25.c                |  2 +-
>  net/xfrm/xfrm_sysctl.c                  |  4 +-
>  64 files changed, 280 insertions(+), 262 deletions(-)
> ---
> base-commit: 0f5cc96c367f2e780eb492cc9cab84e3b2ca88da
> change-id: 20231116-const-sysctl-e14624f1295c
>=20
> Best regards,
> --=20
> Thomas Wei=DFschuh <linux@weissschuh.net>
>=20

--=20

Joel Granados

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

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

iQGzBAABCgAdFiEErkcJVyXmMSXOyyeQupfNUreWQU8FAmVka60ACgkQupfNUreW
QU+Hkgv/fsX1aDSSa8URhpWfgcWFNkt8gE1/YrLUf5bWFX84VcsLpNNFV7ugOfHF
z0U/ZhgJejIQ5VmcDhi8Un86x2YxQ6OQE5LD1Zbk2MKd1W49RGwAR8MRh0qlyHo+
poVDU8p+uf6yPSCUnXLgZlCa+hfohzKy/gbCZw/dQCNoRnfqTDq74vicXvs9pksB
4PM7fRQSyjwibHZeH/LVI2QsgR1rFXcYUHz6AMIYEMCmnQft79TaHwKQXSFLneXW
6H1uyxVsqQIhGhfM7Vxa40bLNyJ6uUoD2Gddf0o5wtALGds8B0aOhQepLbbVWgTw
lX35EnOW9HQg0MW4uR23KUQmMEf15GIdKXO1AJQfWRVKnBCM3U82oTMD0YWjL4oU
m7T/FW87L1eF2xxr+j1RQJqFnLf9Kd4i335UUrcbVKPCzhSQfWFSpy82pw0cq7Wk
4l2yqTSJSB14SzyKBudrciy4rQmF5QAn/SxmbPl9x2svw33umu+N2pjprBiPvSLX
wGXylbx9
=UfF8
-----END PGP SIGNATURE-----

--zn4hpd3vbxzf3eak--