Bug 207626 - [cam] Memory leak in ctl.c
Summary: [cam] Memory leak in ctl.c
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Many People
Assignee: freebsd-bugs (Nobody)
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2016-03-01 20:52 UTC by CTurt
Modified: 2016-05-06 19:13 UTC (History)
6 users (show)

See Also:


Attachments
Patch to fix the memory leak (454 bytes, patch)
2016-03-02 02:37 UTC, Shawn Webb
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 20:52:15 UTC
There is a memory leak in `sys/cam/ctl/ctl.c`.

`ctl_copyin_alloc` performs and returns an allocation with `malloc`:

static void *
ctl_copyin_alloc(void *user_addr, int len, char *error_str,
		 size_t error_str_len)
{
	void *kptr;

	kptr = malloc(len, M_CTL, M_WAITOK | M_ZERO);

	if (copyin(user_addr, kptr, len) != 0) {
		snprintf(error_str, error_str_len, "Error copying %d bytes "
			 "from user address %p to kernel address %p", len,
			 user_addr, kptr);
		free(kptr, M_CTL);
		return (NULL);
	}

	return (kptr);
}

`ctl_copyin_args` calls this function, but doesn't free the returned allocation on the condition that the string is not terminated, before going to `bailout`:

static struct ctl_be_arg *
ctl_copyin_args(int num_args, struct ctl_be_arg *uargs,
		char *error_str, size_t error_str_len)
{
	...
	
	uint8_t *tmpptr;
	
	...

		if (args[i].flags & CTL_BEARG_RD) {
			tmpptr = ctl_copyin_alloc(args[i].value,
				args[i].vallen, error_str, error_str_len);
			if (tmpptr == NULL)
				goto bailout;
			if ((args[i].flags & CTL_BEARG_ASCII)
			 && (tmpptr[args[i].vallen - 1] != '\0')) {
				snprintf(error_str, error_str_len, "Argument "
				    "%d value is not NUL-terminated", i);
				goto bailout;
			}
			args[i].kvalue = tmpptr;
		} else {
			args[i].kvalue = malloc(args[i].vallen,
			    M_CTL, M_WAITOK | M_ZERO);
		}
	}

	return (args);
bailout:

	ctl_free_args(num_args, args);

	return (NULL);
}

Should be:

				snprintf(error_str, error_str_len, "Argument "
				    "%d value is not NUL-terminated", i);
				free(tmpptr);
				goto bailout;
Comment 2 Shawn Webb 2016-03-02 02:37:45 UTC
Created attachment 167620 [details]
Patch to fix the memory leak
Comment 3 Shawn Webb 2016-03-02 02:38:00 UTC
Please attribute the fix to HardenedBSD.
Comment 4 CTurt 2016-04-04 10:24:31 UTC
Shawn, in your patch, setting `tmpptr` to `NULL` after it has been freed is unnecessary since it is out of scope of the `bailout` label, so nothing can use the freed pointer to cause use after free, after the `goto`; however it may be preferable to do so out of good practice.
Comment 5 commit-hook freebsd_committer freebsd_triage 2016-04-19 16:48:53 UTC
A commit references this bug:

Author: sbruno
Date: Tue Apr 19 16:48:15 UTC 2016
New revision: 298279
URL: https://svnweb.freebsd.org/changeset/base/298279

Log:
  Plug memory leak in ctl(4) when ctl_copyin_args() is called with a non-
  null terminated ASCII string.

  PR:		207626
  Submitted by:	cturt@hardenedbsd.org
  MFC after:	2 days

Changes:
  head/sys/cam/ctl/ctl.c
Comment 6 commit-hook freebsd_committer freebsd_triage 2016-05-06 19:12:15 UTC
A commit references this bug:

Author: sbruno
Date: Fri May  6 19:11:48 UTC 2016
New revision: 299191
URL: https://svnweb.freebsd.org/changeset/base/299191

Log:
  MFC r298279

  Plug memory leak in ctl(4) when ctl_copyin_args() is called with a non-
  null terminated ASCII string.

  PR:		207626
  Submitted by:	cturt@hardenedbsd.org

Changes:
_U  stable/10/
  stable/10/sys/cam/ctl/ctl.c