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 <[email protected]>
---
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
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 <[email protected]>
Looks right to me.
Reviewed-by: Kees Cook <[email protected]>
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
Xiaoming Ni <[email protected]> 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" <[email protected]>
> Delete the code and return -ENOSYS directly at the function entry
>
> Signed-off-by: Xiaoming Ni <[email protected]>
> ---
> 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 */
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 <[email protected]>
>
> Looks right to me.
>
> Reviewed-by: Kees Cook <[email protected]>
>
> 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 <[email protected]>" to the v2 patch?
Thanks
Xiaoming Ni
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 <[email protected]>
> >
> > Looks right to me.
> >
> > Reviewed-by: Kees Cook <[email protected]>
> >
> > 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 <[email protected]>" 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
On 2020/6/10 3:20, Eric W. Biederman wrote:
> Xiaoming Ni <[email protected]> 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" <[email protected]>
>
Thanks for your guidance
I will add it in the v2 version
Thanks
Xiaoming Ni