Bug 207627 - [cam] fix negative array index in ctl.c
Summary: [cam] fix negative array index in ctl.c
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Edward Tomasz Napierala
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2016-03-01 21:11 UTC by CTurt
Modified: 2016-12-03 21:27 UTC (History)
8 users (show)

See Also:


Attachments
Proposed patch for #207627 (2.16 KB, patch)
2016-07-16 22:01 UTC, rday
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description CTurt 2016-03-01 21:11:42 UTC
`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.
Comment 1 rday 2016-07-16 22:01:23 UTC
Created attachment 172594 [details]
Proposed patch for #207627
Comment 2 rday 2016-07-16 22:02:37 UTC
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.
Comment 3 Xin LI freebsd_committer 2016-07-20 07:10:48 UTC
Edward can you take a look at this one?
Comment 4 Edward Tomasz Napierala freebsd_committer 2016-08-15 18:31:17 UTC
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?
Comment 5 Xin LI freebsd_committer 2016-08-17 05:54:59 UTC
(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.
Comment 6 commit-hook freebsd_committer 2016-11-03 10:12:13 UTC
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
Comment 7 commit-hook freebsd_committer 2016-12-03 21:27:58 UTC
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