sys_procctl(): Make it clear that negative commands are invalid

An initial reading of the preamble of sys_procctl() gives the impression
that no test prevents a malicious user from passing a negative commands
index (in 'uap->com'), which is soon used as an index into the static
array procctl_cmds_info[].

However, a closer examination leads to the conclusion that the existing
code is technically correct.  Indeed, the comparison of 'uap->com' to
the nitems() expression, which expands to a ratio of sizeof(), leads to
a conversion of 'uap->com' to an 'unsigned int' as per Usual Arithmetic
Conversions/Integer Promotions applied by '<=', because sizeof() returns
'size_t' values, and we define 'size_t' as an equivalent of 'unsigned
int' (which is not mandated by the standard, the latter allowing, e.g.,
integers of lower ranks).

With this conversion, negative values of 'uap->com' are automatically
ruled-out since they are converted to very big unsigned integers which
are caught by the test.  An analysis of assembly code produced by LLVM
16 on amd64 and practical tests confirm that no exploitation is possible.

However, the guard code as written is misleading to readers and might
trip up static analysis tools.  Make sure that negative values are
explicitly excluded so that it is immediately clear that EINVAL will be
returned in this case.

Build tested with clang 16 and GCC 12.

Approved by:    markj (mentor)
MFC after:      1 week
Sponsored by:   The FreeBSD Foundation
This commit is contained in:
Olivier Certner 2024-04-10 16:32:32 +02:00
parent 1d14e88e53
commit afc10f8bba
No known key found for this signature in database
GPG key ID: 8CA13040971E2627

View file

@ -1126,7 +1126,7 @@ sys_procctl(struct thread *td, struct procctl_args *uap)
if (uap->com >= PROC_PROCCTL_MD_MIN)
return (cpu_procctl(td, uap->idtype, uap->id,
uap->com, uap->data));
if (uap->com == 0 || uap->com >= nitems(procctl_cmds_info))
if (uap->com <= 0 || uap->com >= nitems(procctl_cmds_info))
return (EINVAL);
cmd_info = &procctl_cmds_info[uap->com];
bzero(&x, sizeof(x));