Bug 207627
| Summary: | [cam] fix negative array index in ctl.c | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | CTurt <ecturt> | ||||
| Component: | kern | Assignee: | Edward Tomasz Napierala <trasz> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Only Me | CC: | delphij, emaste, mav, op, ryan, secteam, shawn.webb, trasz | ||||
| Priority: | --- | Keywords: | patch | ||||
| Version: | CURRENT | ||||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| Attachments: |
|
||||||
Created attachment 172594 [details]
Proposed patch for #207627
Comment on attachment 172594 [details]
Proposed patch for #207627
After looking over the code I wanted to propose a patch to fix this problem.
If the name length or value length was 0, a negative array index would occur while checking for a NUL terminator. This patch adds a check preventing a 0 name length or value length.
Since the length of value shouldn't be negative, this patch changes the type of namelen and vallen to unsigned.
If memory couldn't be allocated for the argument name or value, the bailout path would attempt to free() a null pointer. This patch adds a check to prevent that from happening.
I also updated some comments detailing which values need a nul termination.
Edward can you take a look at this one? Checking whether the argument is NULL before calling free(9) is pointless, just like in userspace. I'd also prefer the "argument is invalid" replaced by "must be greater than zero", or something like that. Apart from that, the patch looks fine. Have you verified it doesn't break binary compatibility? (In reply to Edward Tomasz Napierala from comment #4) I don't believe the change of signedness would change ABI. Note however that in ctl_copyin_alloc(), the allocation is done with M_WAITOK, which means we could block when the length from userland is sufficiently large. I think we should probably either add a sanity check here to prevent sufficiently large request, or to use M_NOWAIT and bail out as soon as it failed. A commit references this bug: Author: trasz Date: Thu Nov 3 10:11:59 UTC 2016 New revision: 308250 URL: https://svnweb.freebsd.org/changeset/base/308250 Log: Check for lengths being <= 0. Note that this interface can only be accessed by root. It uses unsigned ints instead of size_t to preserve the ABI. PR: 207627 Submitted by: ryan@ryanday.net (with slight tweaks) MFC after: 1 month Changes: head/sys/cam/ctl/ctl.c head/sys/cam/ctl/ctl_ioctl.h A commit references this bug: Author: trasz Date: Sat Dec 3 21:27:19 UTC 2016 New revision: 309516 URL: https://svnweb.freebsd.org/changeset/base/309516 Log: MFC r308250: Check for lengths being <= 0. Note that this interface can only be accessed by root. It uses unsigned ints instead of size_t to preserve the ABI. PR: 207627 Changes: _U stable/11/ stable/11/sys/cam/ctl/ctl.c stable/11/sys/cam/ctl/ctl_ioctl.h Committed back in 2016. |
`struct ctl_be_arg` from `sys/cam/ctl/ctl_ioctl.h` declares its sizes as `signed` integers: struct ctl_be_arg { int namelen; char *name; int flags; int vallen; void *value; char *kname; void *kvalue; }; `ctl_copyin_args` from `sys/cam/ctl/ctl.c` copies these values directly from userland with no bound checks, and then goes on to use them in array indexes: static struct ctl_be_arg * ctl_copyin_args(int num_args, struct ctl_be_arg *uargs, char *error_str, size_t error_str_len) { struct ctl_be_arg *args; int i; args = ctl_copyin_alloc(uargs, num_args * sizeof(*args), error_str, error_str_len); ... for (i = 0; i < num_args; i++) { uint8_t *tmpptr; args[i].kname = ctl_copyin_alloc(args[i].name, args[i].namelen, error_str, error_str_len); ... if (args[i].kname[args[i].namelen - 1] != '\0') { ... if (args[i].flags & CTL_BEARG_RD) { ... && (tmpptr[args[i].vallen - 1] != '\0')) { For example, if a `args[i].namelen` of 0 is supplied, a read will be performed from `args[i].kname[-1]`. Similarly for `tmpptr` and `vallen`. This range could be extended by supplying negative lengths, however this would be less likely to be triggerable since the size is converted to `unsigned` for the allocation and so an allocation of at least 4GB would need to succeed first.