* [PATCH] sysctl: Delete the code of sys_sysctl
@ 2020-06-09 6:20 Xiaoming Ni
2020-06-09 15:40 ` Kees Cook
2020-06-09 19:20 ` Eric W. Biederman
0 siblings, 2 replies; 6+ messages in thread
From: Xiaoming Ni @ 2020-06-09 6:20 UTC (permalink / raw)
To: ebiederm, keescook, ak
Cc: nixiaoming, alex.huangjianhui, linzichang, linux-kernel
Since the commit 61a47c1ad3a4dc ("sysctl: Remove the sysctl system call"),
sys_sysctl has lost its actual role: any input can only return an error.
Delete the code and return -ENOSYS directly at the function entry
Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
---
kernel/sysctl_binary.c | 146 +------------------------------------------------
1 file changed, 2 insertions(+), 144 deletions(-)
diff --git a/kernel/sysctl_binary.c b/kernel/sysctl_binary.c
index 7d550cc..41a88f8 100644
--- a/kernel/sysctl_binary.c
+++ b/kernel/sysctl_binary.c
@@ -1,126 +1,11 @@
// SPDX-License-Identifier: GPL-2.0
-#include <linux/stat.h>
#include <linux/sysctl.h>
-#include "../fs/xfs/xfs_sysctl.h"
-#include <linux/sunrpc/debug.h>
-#include <linux/string.h>
#include <linux/syscalls.h>
-#include <linux/namei.h>
-#include <linux/mount.h>
-#include <linux/fs.h>
-#include <linux/nsproxy.h>
-#include <linux/pid_namespace.h>
-#include <linux/file.h>
-#include <linux/ctype.h>
-#include <linux/netdevice.h>
-#include <linux/kernel.h>
-#include <linux/uuid.h>
-#include <linux/slab.h>
#include <linux/compat.h>
-static ssize_t binary_sysctl(const int *name, int nlen,
- void __user *oldval, size_t oldlen, void __user *newval, size_t newlen)
-{
- return -ENOSYS;
-}
-
-static void deprecated_sysctl_warning(const int *name, int nlen)
-{
- int i;
-
- /*
- * CTL_KERN/KERN_VERSION is used by older glibc and cannot
- * ever go away.
- */
- if (nlen >= 2 && name[0] == CTL_KERN && name[1] == KERN_VERSION)
- return;
-
- if (printk_ratelimit()) {
- printk(KERN_INFO
- "warning: process `%s' used the deprecated sysctl "
- "system call with ", current->comm);
- for (i = 0; i < nlen; i++)
- printk(KERN_CONT "%d.", name[i]);
- printk(KERN_CONT "\n");
- }
- return;
-}
-
-#define WARN_ONCE_HASH_BITS 8
-#define WARN_ONCE_HASH_SIZE (1<<WARN_ONCE_HASH_BITS)
-
-static DECLARE_BITMAP(warn_once_bitmap, WARN_ONCE_HASH_SIZE);
-
-#define FNV32_OFFSET 2166136261U
-#define FNV32_PRIME 0x01000193
-
-/*
- * Print each legacy sysctl (approximately) only once.
- * To avoid making the tables non-const use a external
- * hash-table instead.
- * Worst case hash collision: 6, but very rarely.
- * NOTE! We don't use the SMP-safe bit tests. We simply
- * don't care enough.
- */
-static void warn_on_bintable(const int *name, int nlen)
-{
- int i;
- u32 hash = FNV32_OFFSET;
-
- for (i = 0; i < nlen; i++)
- hash = (hash ^ name[i]) * FNV32_PRIME;
- hash %= WARN_ONCE_HASH_SIZE;
- if (__test_and_set_bit(hash, warn_once_bitmap))
- return;
- deprecated_sysctl_warning(name, nlen);
-}
-
-static ssize_t do_sysctl(int __user *args_name, int nlen,
- void __user *oldval, size_t oldlen, void __user *newval, size_t newlen)
-{
- int name[CTL_MAXNAME];
- int i;
-
- /* Check args->nlen. */
- if (nlen < 0 || nlen > CTL_MAXNAME)
- return -ENOTDIR;
- /* Read in the sysctl name for simplicity */
- for (i = 0; i < nlen; i++)
- if (get_user(name[i], args_name + i))
- return -EFAULT;
-
- warn_on_bintable(name, nlen);
-
- return binary_sysctl(name, nlen, oldval, oldlen, newval, newlen);
-}
-
SYSCALL_DEFINE1(sysctl, struct __sysctl_args __user *, args)
{
- struct __sysctl_args tmp;
- size_t oldlen = 0;
- ssize_t result;
-
- if (copy_from_user(&tmp, args, sizeof(tmp)))
- return -EFAULT;
-
- if (tmp.oldval && !tmp.oldlenp)
- return -EFAULT;
-
- if (tmp.oldlenp && get_user(oldlen, tmp.oldlenp))
- return -EFAULT;
-
- result = do_sysctl(tmp.name, tmp.nlen, tmp.oldval, oldlen,
- tmp.newval, tmp.newlen);
-
- if (result >= 0) {
- oldlen = result;
- result = 0;
- }
-
- if (tmp.oldlenp && put_user(oldlen, tmp.oldlenp))
- return -EFAULT;
-
- return result;
+ return -ENOSYS;
}
@@ -138,34 +23,7 @@ struct compat_sysctl_args {
COMPAT_SYSCALL_DEFINE1(sysctl, struct compat_sysctl_args __user *, args)
{
- struct compat_sysctl_args tmp;
- compat_size_t __user *compat_oldlenp;
- size_t oldlen = 0;
- ssize_t result;
-
- if (copy_from_user(&tmp, args, sizeof(tmp)))
- return -EFAULT;
-
- if (tmp.oldval && !tmp.oldlenp)
- return -EFAULT;
-
- compat_oldlenp = compat_ptr(tmp.oldlenp);
- if (compat_oldlenp && get_user(oldlen, compat_oldlenp))
- return -EFAULT;
-
- result = do_sysctl(compat_ptr(tmp.name), tmp.nlen,
- compat_ptr(tmp.oldval), oldlen,
- compat_ptr(tmp.newval), tmp.newlen);
-
- if (result >= 0) {
- oldlen = result;
- result = 0;
- }
-
- if (compat_oldlenp && put_user(oldlen, compat_oldlenp))
- return -EFAULT;
-
- return result;
+ return -ENOSYS;
}
#endif /* CONFIG_COMPAT */
--
1.8.5.6
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] sysctl: Delete the code of sys_sysctl
2020-06-09 6:20 [PATCH] sysctl: Delete the code of sys_sysctl Xiaoming Ni
@ 2020-06-09 15:40 ` Kees Cook
2020-06-10 14:17 ` Xiaoming Ni
2020-06-09 19:20 ` Eric W. Biederman
1 sibling, 1 reply; 6+ messages in thread
From: Kees Cook @ 2020-06-09 15:40 UTC (permalink / raw)
To: Xiaoming Ni; +Cc: ebiederm, ak, alex.huangjianhui, linzichang, linux-kernel
On Tue, Jun 09, 2020 at 02:20:05PM +0800, Xiaoming Ni wrote:
> Since the commit 61a47c1ad3a4dc ("sysctl: Remove the sysctl system call"),
> sys_sysctl has lost its actual role: any input can only return an error.
>
> Delete the code and return -ENOSYS directly at the function entry
>
> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
Looks right to me.
Reviewed-by: Kees Cook <keescook@chromium.org>
Should this be taken a step further and just remove the syscall entirely
and update the per-arch tables with the ENOSYS hole?
-Kees
--
Kees Cook
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] sysctl: Delete the code of sys_sysctl
2020-06-09 15:40 ` Kees Cook
@ 2020-06-10 14:17 ` Xiaoming Ni
2020-06-10 15:03 ` Kees Cook
0 siblings, 1 reply; 6+ messages in thread
From: Xiaoming Ni @ 2020-06-10 14:17 UTC (permalink / raw)
To: Kees Cook; +Cc: ebiederm, ak, alex.huangjianhui, linzichang, linux-kernel
On 2020/6/9 23:40, Kees Cook wrote:
> On Tue, Jun 09, 2020 at 02:20:05PM +0800, Xiaoming Ni wrote:
>> Since the commit 61a47c1ad3a4dc ("sysctl: Remove the sysctl system call"),
>> sys_sysctl has lost its actual role: any input can only return an error.
>>
>> Delete the code and return -ENOSYS directly at the function entry
>>
>> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
>
> Looks right to me.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> Should this be taken a step further and just remove the syscall entirely
> and update the per-arch tables with the ENOSYS hole?
>
> -Kees
>
Searching for git log, I found a commit record that deleted syscall:
commit f5b94099739722 ("All Arch: remove linkage for sys_nfsservctl
system call"). Could I use sys_ni_syscall to implement the hole as in
the example here?
E.g:
diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
index 7b3832d..f36fda6 100644
--- a/arch/arm/tools/syscall.tbl
+++ b/arch/arm/tools/syscall.tbl
@@ -162,7 +162,7 @@
146 common writev sys_writev
147 common getsid sys_getsid
148 common fdatasync sys_fdatasync
-149 common _sysctl sys_sysctl
+149 common _sysctl sys_ni_syscall
150 common mlock sys_mlock
151 common munlock sys_munlock
152 common mlockall sys_mlockall
diff --git a/arch/arm64/include/asm/unistd32.h
b/arch/arm64/include/asm/unistd32.h
index f8dafe9..ca41bb7 100644
--- a/arch/arm64/include/asm/unistd32.h
+++ b/arch/arm64/include/asm/unistd32.h
@@ -308,8 +308,8 @@
__SYSCALL(__NR_getsid, sys_getsid)
#define __NR_fdatasync 148
__SYSCALL(__NR_fdatasync, sys_fdatasync)
-#define __NR__sysctl 149
-__SYSCALL(__NR__sysctl, compat_sys_sysctl)
+ /* 149 was sys_sysctl */
+__SYSCALL(149, sys_ni_syscall)
#define __NR_mlock 150
__SYSCALL(__NR_mlock, sys_mlock)
#define __NR_munlock 151
In this case, I need to modify a lot of code in v2. Can I add
"Reviewed-by: Kees Cook <keescook@chromium.org>" to the v2 patch?
Thanks
Xiaoming Ni
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] sysctl: Delete the code of sys_sysctl
2020-06-10 14:17 ` Xiaoming Ni
@ 2020-06-10 15:03 ` Kees Cook
0 siblings, 0 replies; 6+ messages in thread
From: Kees Cook @ 2020-06-10 15:03 UTC (permalink / raw)
To: Xiaoming Ni; +Cc: ebiederm, ak, alex.huangjianhui, linzichang, linux-kernel
On Wed, Jun 10, 2020 at 10:17:49PM +0800, Xiaoming Ni wrote:
> On 2020/6/9 23:40, Kees Cook wrote:
> > On Tue, Jun 09, 2020 at 02:20:05PM +0800, Xiaoming Ni wrote:
> > > Since the commit 61a47c1ad3a4dc ("sysctl: Remove the sysctl system call"),
> > > sys_sysctl has lost its actual role: any input can only return an error.
> > >
> > > Delete the code and return -ENOSYS directly at the function entry
> > >
> > > Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> >
> > Looks right to me.
> >
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> >
> > Should this be taken a step further and just remove the syscall entirely
> > and update the per-arch tables with the ENOSYS hole?
> >
> > -Kees
> >
> Searching for git log, I found a commit record that deleted syscall:
> commit f5b94099739722 ("All Arch: remove linkage for sys_nfsservctl system
> call"). Could I use sys_ni_syscall to implement the hole as in the example
> here?
> E.g:
> diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
> index 7b3832d..f36fda6 100644
> --- a/arch/arm/tools/syscall.tbl
> +++ b/arch/arm/tools/syscall.tbl
> @@ -162,7 +162,7 @@
> 146 common writev sys_writev
> 147 common getsid sys_getsid
> 148 common fdatasync sys_fdatasync
> -149 common _sysctl sys_sysctl
> +149 common _sysctl sys_ni_syscall
> 150 common mlock sys_mlock
> 151 common munlock sys_munlock
> 152 common mlockall sys_mlockall
> diff --git a/arch/arm64/include/asm/unistd32.h
> b/arch/arm64/include/asm/unistd32.h
> index f8dafe9..ca41bb7 100644
> --- a/arch/arm64/include/asm/unistd32.h
> +++ b/arch/arm64/include/asm/unistd32.h
> @@ -308,8 +308,8 @@
> __SYSCALL(__NR_getsid, sys_getsid)
> #define __NR_fdatasync 148
> __SYSCALL(__NR_fdatasync, sys_fdatasync)
> -#define __NR__sysctl 149
> -__SYSCALL(__NR__sysctl, compat_sys_sysctl)
> + /* 149 was sys_sysctl */
> +__SYSCALL(149, sys_ni_syscall)
> #define __NR_mlock 150
> __SYSCALL(__NR_mlock, sys_mlock)
> #define __NR_munlock 151
>
>
> In this case, I need to modify a lot of code in v2.
Yeah, that looks like a good example.
> Can I add "Reviewed-by:
> Kees Cook <keescook@chromium.org>" to the v2 patch?
No, it'll be very different. I'm still a fan of the change, but send v2
and I can review that separately. Thanks!
--
Kees Cook
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] sysctl: Delete the code of sys_sysctl
2020-06-09 6:20 [PATCH] sysctl: Delete the code of sys_sysctl Xiaoming Ni
2020-06-09 15:40 ` Kees Cook
@ 2020-06-09 19:20 ` Eric W. Biederman
2020-06-10 14:19 ` Xiaoming Ni
1 sibling, 1 reply; 6+ messages in thread
From: Eric W. Biederman @ 2020-06-09 19:20 UTC (permalink / raw)
To: Xiaoming Ni; +Cc: keescook, ak, alex.huangjianhui, linzichang, linux-kernel
Xiaoming Ni <nixiaoming@huawei.com> writes:
> Since the commit 61a47c1ad3a4dc ("sysctl: Remove the sysctl system call"),
> sys_sysctl has lost its actual role: any input can only return an error.
The remaining code does have a role. It reports programs that attempt
to use the removed sysctl.
It would help if your change description had a reason why we don't want
to warn people that a program has used a removed system call.
Probably something like:
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.
With a change like that made to the patch description.
Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> Delete the code and return -ENOSYS directly at the function entry
>
> Signed-off-by: Xiaoming Ni <nixiaoming@huawei.com>
> ---
> kernel/sysctl_binary.c | 146 +------------------------------------------------
> 1 file changed, 2 insertions(+), 144 deletions(-)
>
> diff --git a/kernel/sysctl_binary.c b/kernel/sysctl_binary.c
> index 7d550cc..41a88f8 100644
> --- a/kernel/sysctl_binary.c
> +++ b/kernel/sysctl_binary.c
> @@ -1,126 +1,11 @@
> // SPDX-License-Identifier: GPL-2.0
> -#include <linux/stat.h>
> #include <linux/sysctl.h>
> -#include "../fs/xfs/xfs_sysctl.h"
> -#include <linux/sunrpc/debug.h>
> -#include <linux/string.h>
> #include <linux/syscalls.h>
> -#include <linux/namei.h>
> -#include <linux/mount.h>
> -#include <linux/fs.h>
> -#include <linux/nsproxy.h>
> -#include <linux/pid_namespace.h>
> -#include <linux/file.h>
> -#include <linux/ctype.h>
> -#include <linux/netdevice.h>
> -#include <linux/kernel.h>
> -#include <linux/uuid.h>
> -#include <linux/slab.h>
> #include <linux/compat.h>
>
> -static ssize_t binary_sysctl(const int *name, int nlen,
> - void __user *oldval, size_t oldlen, void __user *newval, size_t newlen)
> -{
> - return -ENOSYS;
> -}
> -
> -static void deprecated_sysctl_warning(const int *name, int nlen)
> -{
> - int i;
> -
> - /*
> - * CTL_KERN/KERN_VERSION is used by older glibc and cannot
> - * ever go away.
> - */
> - if (nlen >= 2 && name[0] == CTL_KERN && name[1] == KERN_VERSION)
> - return;
> -
> - if (printk_ratelimit()) {
> - printk(KERN_INFO
> - "warning: process `%s' used the deprecated sysctl "
> - "system call with ", current->comm);
> - for (i = 0; i < nlen; i++)
> - printk(KERN_CONT "%d.", name[i]);
> - printk(KERN_CONT "\n");
> - }
> - return;
> -}
> -
> -#define WARN_ONCE_HASH_BITS 8
> -#define WARN_ONCE_HASH_SIZE (1<<WARN_ONCE_HASH_BITS)
> -
> -static DECLARE_BITMAP(warn_once_bitmap, WARN_ONCE_HASH_SIZE);
> -
> -#define FNV32_OFFSET 2166136261U
> -#define FNV32_PRIME 0x01000193
> -
> -/*
> - * Print each legacy sysctl (approximately) only once.
> - * To avoid making the tables non-const use a external
> - * hash-table instead.
> - * Worst case hash collision: 6, but very rarely.
> - * NOTE! We don't use the SMP-safe bit tests. We simply
> - * don't care enough.
> - */
> -static void warn_on_bintable(const int *name, int nlen)
> -{
> - int i;
> - u32 hash = FNV32_OFFSET;
> -
> - for (i = 0; i < nlen; i++)
> - hash = (hash ^ name[i]) * FNV32_PRIME;
> - hash %= WARN_ONCE_HASH_SIZE;
> - if (__test_and_set_bit(hash, warn_once_bitmap))
> - return;
> - deprecated_sysctl_warning(name, nlen);
> -}
> -
> -static ssize_t do_sysctl(int __user *args_name, int nlen,
> - void __user *oldval, size_t oldlen, void __user *newval, size_t newlen)
> -{
> - int name[CTL_MAXNAME];
> - int i;
> -
> - /* Check args->nlen. */
> - if (nlen < 0 || nlen > CTL_MAXNAME)
> - return -ENOTDIR;
> - /* Read in the sysctl name for simplicity */
> - for (i = 0; i < nlen; i++)
> - if (get_user(name[i], args_name + i))
> - return -EFAULT;
> -
> - warn_on_bintable(name, nlen);
> -
> - return binary_sysctl(name, nlen, oldval, oldlen, newval, newlen);
> -}
> -
> SYSCALL_DEFINE1(sysctl, struct __sysctl_args __user *, args)
> {
> - struct __sysctl_args tmp;
> - size_t oldlen = 0;
> - ssize_t result;
> -
> - if (copy_from_user(&tmp, args, sizeof(tmp)))
> - return -EFAULT;
> -
> - if (tmp.oldval && !tmp.oldlenp)
> - return -EFAULT;
> -
> - if (tmp.oldlenp && get_user(oldlen, tmp.oldlenp))
> - return -EFAULT;
> -
> - result = do_sysctl(tmp.name, tmp.nlen, tmp.oldval, oldlen,
> - tmp.newval, tmp.newlen);
> -
> - if (result >= 0) {
> - oldlen = result;
> - result = 0;
> - }
> -
> - if (tmp.oldlenp && put_user(oldlen, tmp.oldlenp))
> - return -EFAULT;
> -
> - return result;
> + return -ENOSYS;
> }
>
>
> @@ -138,34 +23,7 @@ struct compat_sysctl_args {
>
> COMPAT_SYSCALL_DEFINE1(sysctl, struct compat_sysctl_args __user *, args)
> {
> - struct compat_sysctl_args tmp;
> - compat_size_t __user *compat_oldlenp;
> - size_t oldlen = 0;
> - ssize_t result;
> -
> - if (copy_from_user(&tmp, args, sizeof(tmp)))
> - return -EFAULT;
> -
> - if (tmp.oldval && !tmp.oldlenp)
> - return -EFAULT;
> -
> - compat_oldlenp = compat_ptr(tmp.oldlenp);
> - if (compat_oldlenp && get_user(oldlen, compat_oldlenp))
> - return -EFAULT;
> -
> - result = do_sysctl(compat_ptr(tmp.name), tmp.nlen,
> - compat_ptr(tmp.oldval), oldlen,
> - compat_ptr(tmp.newval), tmp.newlen);
> -
> - if (result >= 0) {
> - oldlen = result;
> - result = 0;
> - }
> -
> - if (compat_oldlenp && put_user(oldlen, compat_oldlenp))
> - return -EFAULT;
> -
> - return result;
> + return -ENOSYS;
> }
>
> #endif /* CONFIG_COMPAT */
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] sysctl: Delete the code of sys_sysctl
2020-06-09 19:20 ` Eric W. Biederman
@ 2020-06-10 14:19 ` Xiaoming Ni
0 siblings, 0 replies; 6+ messages in thread
From: Xiaoming Ni @ 2020-06-10 14:19 UTC (permalink / raw)
To: Eric W. Biederman
Cc: keescook, ak, alex.huangjianhui, linzichang, linux-kernel
On 2020/6/10 3:20, Eric W. Biederman wrote:
> Xiaoming Ni <nixiaoming@huawei.com> writes:
>
>> Since the commit 61a47c1ad3a4dc ("sysctl: Remove the sysctl system call"),
>> sys_sysctl has lost its actual role: any input can only return an error.
>
> The remaining code does have a role. It reports programs that attempt
> to use the removed sysctl.
>
> It would help if your change description had a reason why we don't want
> to warn people that a program has used a removed system call.
>
> Probably something like:
>
> 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.
>
> With a change like that made to the patch description.
>
> Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
Thanks for your guidance
I will add it in the v2 version
Thanks
Xiaoming Ni
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-06-10 15:03 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-09 6:20 [PATCH] sysctl: Delete the code of sys_sysctl Xiaoming Ni
2020-06-09 15:40 ` Kees Cook
2020-06-10 14:17 ` Xiaoming Ni
2020-06-10 15:03 ` Kees Cook
2020-06-09 19:20 ` Eric W. Biederman
2020-06-10 14:19 ` Xiaoming Ni
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).