On Fri, Feb 10, 2023 at 03:58:23PM +0100, Ondrej Mosnacek wrote:
> Currently proc_dobool expects a (bool *) in table->data, but sizeof(int)
> in table->maxsize, because it uses do_proc_dointvec() directly.
>
> This is unsafe for at least two reasons:
> 1. A sysctl table definition may use { .data = &variable, .maxsize =
> sizeof(variable) }, not realizing that this makes the sysctl unusable
> (see the Fixes: tag) and that they need to use the completely
> counterintuitive sizeof(int) instead.
> 2. proc_dobool() will currently try to parse an array of values if given
> .maxsize >= 2*sizeof(int), but will try to write values of type bool
> by offsets of sizeof(int), so it will not work correctly with neither
> an (int *) nor a (bool *). There is no .maxsize validation to prevent
> this.
>
> Fix this by:
> 1. Constraining proc_dobool() to allow only one value and .maxsize ==
> sizeof(bool).
> 2. Wrapping the original struct ctl_table in a temporary one with .data
> pointing to a local int variable and .maxsize set to sizeof(int) and
> passing this one to proc_dointvec(), converting the value to/from
> bool as needed (using proc_dou8vec_minmax() as an example).
> 3. Extending sysctl_check_table() to enforce proc_dobool() expectations.
> 4. Fixing the proc_dobool() docstring (it was just copy-pasted from
> proc_douintvec, apparently...).
> 5. Converting all existing proc_dobool() users to set .maxsize to
> sizeof(bool) instead of sizeof(int).
>
> Fixes: 83efeeeb3d04 ("tty: Allow TIOCSTI to be disabled")
> Fixes: a2071573d634 ("sysctl: introduce new proc handler proc_dobool")
> Signed-off-by: Ondrej Mosnacek <omosnace@xxxxxxxxxx>
Ah nice, thanks for tracking this down.
Acked-by: Kees Cook <keescook@xxxxxxxxxxxx>
I've long wanted to replace all the open-coded sysctl initializers with
a helper macro so it's hard to make mistakes. e.g.
#define _SYSCTL_HANDLER(var) \
_Generic((var), \
default: proc_dointvec_minmax, \
unsigned int: proc_douintvec_minmax, \
unsigned long: proc_doulongvec_minmax, \
char: proc_dou8vec_minmax, \
bool: proc_dobool, \
char *: proc_dostring)
#define _SYSCTL_MINVAL(var) \
_Generic((var), \
default: 0, \
int: INT_MIN, \
unsigned int: UINT_MIN, \
unsigned long: ULONG_MIN, \
char: U8_MIN, \
bool: 0)
#define _SYSCTL_MAXVAL(var) \
_Generic((var), \
default: 0, \
int: INT_MAX, \
unsigned int: UINT_MAX, \
unsigned long: ULONG_MAX, \
char: U8_MAX, \
bool: 1)
#define _SYSCTL_VAR(_name, _var, _mode, _handler, _len, _min, _max) \
{ \
.procname = _name, \
.data = &(_var), \
.maxlen = _len, \
.mode = _mode, \
.proc_handler = _handler, \
.minlen = _min, \
.maxlen = _min, \
}
/* single value */
#define SYSCTL_VAL(var) \
_SYSCTL_VAR(#var, var, 0644, _SYSCTL_HANDLER(var), sizeof(var), \
_SYSCTL_MINVAL(var), _SYSCTL_MAXVAL(var))
/* single value with min/max */
#define SYSCTL_VAL_MINMAX(_var, _min, _max) \
_SYSCTL_VAR(#var, var, 0644, _SYSCTL_HANDLER(var), sizeof(var), \
_min, _max)
/* Strings... */
#define SYSCTL_STR(_var, _len) \
_SYSCTL_VAR(#var, var, 0644, _SYSCTL_HANDLER(var), _len, 0, 0)
/* Arrays... */
#define SYSCTL_ARRAY(_var) \
_SYSCTL_VAR(#var, var, 0644, _SYSCTL_HANDLER(var), \
ARRAY_SIZE(_var), _SYSCTL_MINVAL(var), \
_SYSCTL_MAXVAL(var)
Totally untested. I think this would cover the vast majority of sysctl
initializers.
--
Kees Cook
