Re: [PATCH v2] All arch: remove system call sys_sysctl

[ebiederm@xmission.com (Eric W. Biederman)]

Rich Felker <dalias@libc.org> writes:

> On Thu, Jun 11, 2020 at 06:43:00AM -0500, Eric W. Biederman wrote:
>> Xiaoming Ni <nixiaoming@huawei.com> writes:
>> 
>> > Since the commit 61a47c1ad3a4dc ("sysctl: Remove the sysctl system call"),
>> > sys_sysctl is actually unavailable: any input can only return an error.
>> >
>> > We have been warning about people using the sysctl system call for years
>> > and believe there are no more users.  Even if there are users of this
>> > interface if they have not complained or fixed their code by now they
>> > probably are not going to, so there is no point in warning them any
>> > longer.
>> >
>> > So completely remove sys_sysctl on all architectures.
>> 
>> 
>> 
>> >
>> > Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
>> >
>> > changes in v2:
>> >   According to Kees Cook's suggestion, completely remove sys_sysctl on all arch
>> >   According to Eric W. Biederman's suggestion, update the commit log
>> >
>> > V1: https://lore.kernel.org/lkml/1591683605-8585-1-git-send-email-nixiaoming@huawei.com/
>> >   Delete the code of sys_sysctl and return -ENOSYS directly at the function entry
>> > ---
>> >  include/uapi/linux/sysctl.h                        |  15 --
>> [snip]
>> 
>> > diff --git a/include/uapi/linux/sysctl.h b/include/uapi/linux/sysctl.h
>> > index 27c1ed2..84b44c3 100644
>> > --- a/include/uapi/linux/sysctl.h
>> > +++ b/include/uapi/linux/sysctl.h
>> > @@ -27,21 +27,6 @@
>> >  #include <linux/types.h>
>> >  #include <linux/compiler.h>
>> >  
>> > -#define CTL_MAXNAME 10		/* how many path components do we allow in a
>> > -				   call to sysctl?   In other words, what is
>> > -				   the largest acceptable value for the nlen
>> > -				   member of a struct __sysctl_args to have? */
>> > -
>> > -struct __sysctl_args {
>> > -	int __user *name;
>> > -	int nlen;
>> > -	void __user *oldval;
>> > -	size_t __user *oldlenp;
>> > -	void __user *newval;
>> > -	size_t newlen;
>> > -	unsigned long __unused[4];
>> > -};
>> > -
>> >  /* Define sysctl names first */
>> >  
>> >  /* Top-level names: */
>> [snip]
>> 
>> The uapi header change does not make sense.  The entire point of the
>> header is to allow userspace programs to be able to call sys_sysctl.
>> It either needs to all stay or all go.
>> 
>> As the concern with the uapi header is about userspace programs being
>> able to compile please leave the header for now.
>> 
>> We should leave auditing userspace and seeing if userspace code will
>> still compile if we remove this header for a separate patch.  The
>> concerns and justifications for the uapi header are completely different
>> then for the removing the sys_sysctl implementation.
>> 
>> Otherwise
>> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
> The UAPI header should be kept because it's defining an API not just
> for the kernel the headers are supplied with, but for all past
> kernels. In particular programs needing a failsafe CSPRNG source that
> works on old kernels may (do) use this as a fallback only if modern
> syscalls are missing. Removing the syscall is no problem since it
> won't be used, but if you remove the types/macros from the UAPI
> headers, they'll have to copy that into their own sources.

May we assume you know of a least one piece of userspace that will fail
to compile if this header file is removed?

Eric