Bug 206754

Summary: Out of bounds negative array index in iicrdwr
Product: Base System Reporter: CTurt <ecturt>
Component: kernAssignee: freebsd-bugs (Nobody) <bugs>
Status: Closed FIXED    
Severity: Affects Only Me CC: emaste, gonzo, mmokhi, ngie, op
Priority: --- Keywords: patch, patch-ready, security
Version: CURRENTFlags: mmokhi: mfc-stable10?
mmokhi: mfc-stable9?
Hardware: Any   
OS: Any   
URL: https://reviews.freebsd.org/D5132
See Also: https://reviews.freebsd.org/D5155

Description CTurt 2016-01-30 09:33:55 UTC
`iicrdwr` in `/sys/dev/iicbus/iic.c` incorrectly handles iteration over buffer.

Firstly, no bound checks are supplied on the user controlled `d->nmsgs`.

This field is declared as type `uint32_t`, in `struct iic_rdwr_data` (`sys/dev/iicbus/iic.h`):

struct iic_rdwr_data {
	struct iic_msg *msgs;
	uint32_t nmsgs;
};

However, the `i` variable in this function is declared as a `signed int`:

int error, i;

When `i` iterates over buffers, since it is `signed`, it can wrap around to a negative value, for example here:

	for (i = 0; i < d->nmsgs; i++) {
		m = &(buf[i]);
		usrbufs[i] = m->buf;

And here:

	for (i = 0; i < d->nmsgs; i++) {
		m = &(buf[i]);
		if ((error == 0) && (m->flags & IIC_M_RD))
			error = copyout(m->buf, usrbufs[i], m->len);
		free(m->buf, M_IIC);
	}

`i` will be converted to `unsigned` type for the conversion, however, will still be `signed` when indexing `buf`. This would result in a read out of bounds of the `buf` allocation.

This situation seems unlikely to be triggerable, because the code would wait for `buf` allocation to succeed (`M_WAITOK`):

buf = malloc(sizeof(*d->msgs) * d->nmsgs, M_IIC, M_WAITOK);

Which would be unlikely to succeed if `d->nmsgs` is something like `0x80000001`.
Comment 1 CTurt 2016-01-30 09:36:46 UTC
Potential patch is to change the type of `i` from `int` to `uint32_t` to match that of `d->nmsgs`:

https://github.com/HardenedBSD/hardenedBSD-playground/commit/4cf6de4b16eda71c4ae3cbec24cf6ef054351b7b.patch

However, some bound checks might be a cleaner solution.
Comment 2 Mahdi Mokhtari freebsd_committer freebsd_triage 2016-01-30 14:24:18 UTC
Ive made patch based on CTurt's for this and issue#206755
waiting to be reviewed and approved.
Comment 3 commit-hook freebsd_committer freebsd_triage 2016-01-30 18:34:04 UTC
A commit references this bug:

Author: ngie
Date: Sat Jan 30 18:33:24 UTC 2016
New revision: 295080
URL: https://svnweb.freebsd.org/changeset/base/295080

Log:
  Use the correct type for i when iterating over `buf` to avoid unlikely
  negative array indexing in iicrdwr(..)

  Differential Revision: https://reviews.freebsd.org/D5132
  Obtained from: HardenedBSD
  PR: 206754
  Reported by: CTurt <cturt@hardenedbsd.org>
  Submitted by: Madhi Moktari <mokhi64@gmail.com>
  Sponsored by: EMC / Isilon Storage Division

Changes:
  head/sys/dev/iicbus/iic.c
Comment 4 Enji Cooper freebsd_committer freebsd_triage 2016-01-30 18:35:03 UTC
Thanks for the submission! Will work on MFCing back to stable/10 and stable/9 as needed.
Comment 5 Mahdi Mokhtari freebsd_committer freebsd_triage 2016-03-06 21:32:54 UTC
(In reply to Ngie Cooper from comment #4)
@Ngie: will it be MFC'ed ?
Comment 6 Enji Cooper freebsd_committer freebsd_triage 2016-03-28 05:45:46 UTC
(In reply to Mahdi Mokhtari from comment #5)

This item hasn't been MFCed yet due to other QA issues with the module. Will open another bug with more info.
Comment 7 Mahdi Mokhtari freebsd_committer freebsd_triage 2016-03-28 08:40:24 UTC
(In reply to Ngie Cooper from comment #6)
Okay.
Thanks for assigning it to yourself :)

Then, please make this issue depend to that one you'll create.
Comment 8 Enji Cooper freebsd_committer freebsd_triage 2017-01-22 22:04:57 UTC
Untaking bug -- I don't have the hardware to test out this component and this driver has other architectural issues that I don't want to get stuck dealing with.
Comment 9 Oleksandr Tymoshenko freebsd_committer freebsd_triage 2019-01-15 23:55:52 UTC
The fix was committed as base r300258. Closing as Fixed