Bug 206573 - Improper userland pointer handling in aacraid
Summary: Improper userland pointer handling in aacraid
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: misc (show other bugs)
Version: CURRENT
Hardware: Any Any
: Normal Affects Some People
Assignee: FreeBSD bugs mailing list
URL: https://github.com/HardenedBSD/harden...
Keywords: needs-qa, patch, security
Depends on:
Blocks:
 
Reported: 2016-01-24 13:44 UTC by CTurt
Modified: 2016-05-06 19:20 UTC (History)
8 users (show)

See Also:
koobs: mfc-stable9?
koobs: mfc-stable10?


Attachments
unified patch for aac and aacraid (2.40 KB, patch)
2016-04-19 17:30 UTC, Sean Bruno
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description CTurt 2016-01-24 13:44:32 UTC
The `aac_ioctl_send_raw_srb` function can be reached by supplying the `FSACTL_LNX_SEND_RAW_SRB` command to `aac_ioctl`. This code path dereferences a user supplied pointer directly:

static int
aac_ioctl_send_raw_srb(struct aac_softc *sc, caddr_t arg)
{
	struct aac_srb *user_srb = (struct aac_srb *)arg;
	
	...
	
	if ((error = copyin((void *)user_srb, srbcmd, fibsize) != 0)) 
		goto out;
	
	...
	
	struct aac_sg_entry *sgp = srbcmd->sg_map.SgEntry;
	
	...
	
	srb_sg_bytecount = sgp->SgByteCount;
	
	...
}

`srbcmd` has user controlled contents (after `copyin` from `user_srb`).

`sgp` is set to a user controlled address (`srbcmd->sg_map.SgEntry`).

`sgp` is then dereferenced numerous times (`sgp->SgByteCount`).

One impact of this improper handling is that `sgp` could be `NULL`, which would result in a `NULL` dereference, and panic.
Comment 2 landaire 2016-02-02 18:20:50 UTC
This bug is also present in the `aac` (not aacraid) code in the same function:

here:

/* Retrieve correct SG entries. */
if (fibsize == (sizeof(struct aac_srb) +
	   srbcmd->sg_map.SgCount * sizeof(struct aac_sg_entry))) {
	sge = srbcmd->sg_map.SgEntry;
	sge64 = NULL;
	srb_sg_bytecount = sge->SgByteCount;
	srb_sg_address = (void *)(uintptr_t)sge->SgAddress;
}

and here:

https://github.com/freebsd/freebsd/blob/bac8688b17d735d252ec75a94df67384938f3f9b/sys/dev/aac/aac.c#L3114-L3122

#ifdef __amd64__
else if (fibsize == (sizeof(struct aac_srb) +
    srbcmd->sg_map.SgCount * sizeof(struct aac_sg_entry64))) {
	sge = NULL;
	sge64 = (struct aac_sg_entry64 *)srbcmd->sg_map.SgEntry;
	srb_sg_bytecount = sge64->SgByteCount;

	...
}
#endif
Comment 3 landaire 2016-02-02 18:21:45 UTC
Removed the link for the first chunk by accident: https://github.com/freebsd/freebsd/blob/bac8688b17d735d252ec75a94df67384938f3f9b/sys/dev/aac/aac.c#L3104-L3110
Comment 4 Shawn Webb 2016-03-01 20:56:52 UTC
Any movement on this?
Comment 5 op 2016-04-09 12:17:40 UTC
Any update with this issue?!
Comment 6 op 2016-04-09 12:18:46 UTC
Cturt: have you finished the exploit for this issue?
Comment 7 Sean Bruno freebsd_committer 2016-04-19 17:30:11 UTC
Created attachment 169475 [details]
unified patch for aac and aacraid

Ok, this is what I came up with for aac that duplicates the behavior of aacraid patch from cturt.

Comments?
Comment 8 CTurt 2016-04-19 17:43:10 UTC
(In reply to Sean Bruno from comment #7)
This patch looks good to me. Thanks, Sean.
Comment 9 commit-hook freebsd_committer 2016-04-19 18:28:03 UTC
A commit references this bug:

Author: sbruno
Date: Tue Apr 19 18:27:29 UTC 2016
New revision: 298280
URL: https://svnweb.freebsd.org/changeset/base/298280

Log:
  aacraid(4): Sanely copyin userland pointers and ensure that we don't get
  anything janky from a user. (cturt)

  aac(4): landergriffith+freebsdbugzilla@gmail.com pointed out that aacraid(4)
  had the same issue and handling of pointers, so let's change that too.

  PR:		206573
  Submitted by:	cturt@hardenedbsd.org
  Obtained from:	HardenedBSD
  MFC after:	1 week

Changes:
  head/sys/dev/aac/aac.c
  head/sys/dev/aacraid/aacraid.c
Comment 10 commit-hook freebsd_committer 2016-05-06 19:19:20 UTC
A commit references this bug:

Author: sbruno
Date: Fri May  6 19:18:45 UTC 2016
New revision: 299193
URL: https://svnweb.freebsd.org/changeset/base/299193

Log:
  MFC r298280

  aacraid(4): Sanely copyin userland pointers and ensure that we don't get
  anything janky from a user. (cturt)

  aac(4) landergriffith+freebsdbugzilla@gmail.com pointed out that aac(4)
  had the same issue and handling of pointers, so let's change that too.

  PR:		206573
  Submitted by:	cturt@hardenedbsd.org

Changes:
_U  stable/10/
  stable/10/sys/dev/aac/aac.c
  stable/10/sys/dev/aacraid/aacraid.c