On Sun, Jan 23, 2022 at 10:29:22PM -0800, Greg Steuck wrote: > As I was staring a bit more at sysctl related code this pattern caught > my attention. Looks like a few lines can disappear and hopefully code > expressivity goes up. > > Anybody like this?
OK bluhm@ > >From 3b52f9ad743fe9b59316077478d33f77fff9a119 Mon Sep 17 00:00:00 2001 > From: Greg Steuck <[email protected]> > Date: Sun, 23 Jan 2022 22:05:45 -0800 > Subject: [PATCH] Capture a repeated pattern into sysctl_securelevel_int > function > > A few variables in the kernel are only writeable before securelevel is > raised. It makes sense to handle them with less code. > --- > sys/arch/amd64/amd64/machdep.c | 8 ++------ > sys/arch/i386/i386/machdep.c | 8 ++------ > sys/kern/kern_sysctl.c | 33 +++++++++++++++++++-------------- > sys/kern/vfs_subr.c | 5 ++--- > sys/netinet/ip_input.c | 7 +------ > sys/sys/sysctl.h | 1 + > 6 files changed, 27 insertions(+), 35 deletions(-) > > diff --git a/sys/arch/amd64/amd64/machdep.c b/sys/arch/amd64/amd64/machdep.c > index 326f1c10253..4f9585510a1 100644 > --- a/sys/arch/amd64/amd64/machdep.c > +++ b/sys/arch/amd64/amd64/machdep.c > @@ -513,12 +513,8 @@ cpu_sysctl(int *name, u_int namelen, void *oldp, size_t > *oldlenp, void *newp, > case CPU_CPUVENDOR: > return (sysctl_rdstring(oldp, oldlenp, newp, cpu_vendor)); > case CPU_KBDRESET: > - if (securelevel > 0) > - return (sysctl_rdint(oldp, oldlenp, newp, > - kbd_reset)); > - else > - return (sysctl_int(oldp, oldlenp, newp, newlen, > - &kbd_reset)); > + return (sysctl_securelevel_int(oldp, oldlenp, newp, newlen, > + &kbd_reset)); > case CPU_ALLOWAPERTURE: > if (namelen != 1) > return (ENOTDIR); /* overloaded */ > diff --git a/sys/arch/i386/i386/machdep.c b/sys/arch/i386/i386/machdep.c > index 0d32cd4edd7..56e30fac9df 100644 > --- a/sys/arch/i386/i386/machdep.c > +++ b/sys/arch/i386/i386/machdep.c > @@ -3617,12 +3617,8 @@ cpu_sysctl(int *name, u_int namelen, void *oldp, > size_t *oldlenp, void *newp, > case CPU_CPUFEATURE: > return (sysctl_rdint(oldp, oldlenp, newp, > curcpu()->ci_feature_flags)); > case CPU_KBDRESET: > - if (securelevel > 0) > - return (sysctl_rdint(oldp, oldlenp, newp, > - kbd_reset)); > - else > - return (sysctl_int(oldp, oldlenp, newp, newlen, > - &kbd_reset)); > + return (sysctl_securelevel_int(oldp, oldlenp, newp, newlen, > + &kbd_reset)); > #if NPCKBC > 0 && NUKBD > 0 > case CPU_FORCEUKBD: > { > diff --git a/sys/kern/kern_sysctl.c b/sys/kern/kern_sysctl.c > index e71cdf5897d..fe6a2689365 100644 > --- a/sys/kern/kern_sysctl.c > +++ b/sys/kern/kern_sysctl.c > @@ -473,14 +473,12 @@ kern_sysctl(int *name, u_int namelen, void *oldp, > size_t *oldlenp, void *newp, > return (0); > #if NDT > 0 > case KERN_ALLOWDT: > - if (securelevel > 0) > - return (sysctl_rdint(oldp, oldlenp, newp, allowdt)); > - return (sysctl_int(oldp, oldlenp, newp, newlen, &allowdt)); > + return (sysctl_securelevel_int(oldp, oldlenp, newp, newlen, > + &allowdt)); > #endif > case KERN_ALLOWKMEM: > - if (securelevel > 0) > - return (sysctl_rdint(oldp, oldlenp, newp, allowkmem)); > - return (sysctl_int(oldp, oldlenp, newp, newlen, &allowkmem)); > + return (sysctl_securelevel_int(oldp, oldlenp, newp, newlen, > + &allowkmem)); > case KERN_HOSTNAME: > error = sysctl_tstring(oldp, oldlenp, newp, newlen, > hostname, sizeof(hostname)); > @@ -757,10 +755,7 @@ hw_sysctl(int *name, u_int namelen, void *oldp, size_t > *oldlenp, void *newp, > return (sysctl_rdquad(oldp, oldlenp, newp, > ptoa((psize_t)physmem - uvmexp.wired))); > case HW_ALLOWPOWERDOWN: > - if (securelevel > 0) > - return (sysctl_rdint(oldp, oldlenp, newp, > - allowpowerdown)); > - return (sysctl_int(oldp, oldlenp, newp, newlen, > + return (sysctl_securelevel_int(oldp, oldlenp, newp, newlen, > &allowpowerdown)); > #ifdef __HAVE_CPU_TOPOLOGY > case HW_SMT: > @@ -875,6 +870,18 @@ sysctl_rdint(void *oldp, size_t *oldlenp, void *newp, > int val) > return (error); > } > > +/* > + * Selects between sysctl_rdint and sysctl_int according to securelevel. > + */ > +int > +sysctl_securelevel_int(void *oldp, size_t *oldlenp, void *newp, size_t > newlen, > + int *valp) > +{ > + if (securelevel > 0) > + return (sysctl_rdint(oldp, oldlenp, newp, *valp)); > + return (sysctl_int(oldp, oldlenp, newp, newlen, valp)); > +} > + > /* > * Read-only or bounded integer values. > */ > @@ -2501,11 +2508,9 @@ sysctl_utc_offset(void *oldp, size_t *oldlenp, void > *newp, size_t newlen) > int adjustment_seconds, error, new_offset_minutes, old_offset_minutes; > > old_offset_minutes = utc_offset / 60; /* seconds -> minutes */ > - if (securelevel > 0) > - return sysctl_rdint(oldp, oldlenp, newp, old_offset_minutes); > - > new_offset_minutes = old_offset_minutes; > - error = sysctl_int(oldp, oldlenp, newp, newlen, &new_offset_minutes); > + error = sysctl_securelevel_int(oldp, oldlenp, newp, newlen, > + &new_offset_minutes); > if (error) > return error; > if (new_offset_minutes < -24 * 60 || new_offset_minutes > 24 * 60) > diff --git a/sys/kern/vfs_subr.c b/sys/kern/vfs_subr.c > index 43a7cbd4ae9..a60eb0397d7 100644 > --- a/sys/kern/vfs_subr.c > +++ b/sys/kern/vfs_subr.c > @@ -1845,9 +1845,8 @@ fs_posix_sysctl(int *name, u_int namelen, void *oldp, > size_t *oldlenp, > > switch (name[0]) { > case FS_POSIX_SETUID: > - if (newp && securelevel > 0) > - return (EPERM); > - return(sysctl_int(oldp, oldlenp, newp, newlen, &suid_clear)); > + return (sysctl_securelevel_int(oldp, oldlenp, newp, newlen, > + &suid_clear)); > default: > return (EOPNOTSUPP); > } > diff --git a/sys/netinet/ip_input.c b/sys/netinet/ip_input.c > index 9e7e20af30b..2956f013f9f 100644 > --- a/sys/netinet/ip_input.c > +++ b/sys/netinet/ip_input.c > @@ -1610,13 +1610,8 @@ ip_sysctl(int *name, u_int namelen, void *oldp, size_t > *oldlenp, void *newp, > > switch (name[0]) { > case IPCTL_SOURCEROUTE: > - /* > - * Don't allow this to change in a secure environment. > - */ > - if (newp && securelevel > 0) > - return (EPERM); > NET_LOCK(); > - error = sysctl_int(oldp, oldlenp, newp, newlen, > + error = sysctl_securelevel_int(oldp, oldlenp, newp, newlen, > &ip_dosourceroute); > NET_UNLOCK(); > return (error); > diff --git a/sys/sys/sysctl.h b/sys/sys/sysctl.h > index 29f82bcece3..b8e9b1d13ac 100644 > --- a/sys/sys/sysctl.h > +++ b/sys/sys/sysctl.h > @@ -1017,6 +1017,7 @@ typedef int (sysctlfn)(int *, u_int, void *, size_t *, > void *, size_t, struct pr > int sysctl_int_lower(void *, size_t *, void *, size_t, int *); > int sysctl_int(void *, size_t *, void *, size_t, int *); > int sysctl_rdint(void *, size_t *, void *, int); > +int sysctl_securelevel_int(void *, size_t *, void *, size_t, int *); > int sysctl_int_bounded(void *, size_t *, void *, size_t, int *, int, int); > int sysctl_bounded_arr(const struct sysctl_bounded_args *, u_int, > int *, u_int, void *, size_t *, void *, size_t); > -- > 2.34.1
