Bug 209661 - amd64_set_ioperm overflow
Summary: amd64_set_ioperm overflow
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: amd64 Any
: --- Affects Only Me
Assignee: FreeBSD bugs mailing list
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-05-20 13:44 UTC by CTurt
Modified: 2016-06-16 05:07 UTC (History)
4 users (show)

See Also:


Attachments
amd64 + i386 overflow check (1.24 KB, patch)
2016-05-20 19:33 UTC, Konstantin Belousov
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description CTurt 2016-05-20 13:44:55 UTC
The privileged `sysarch` handler, `amd64_set_ioperm`, performs an incorrect bound check on user arguments supplied to it.

The `uap->start + uap->length > ...` check can be bypassed if the two user controlled values overflow when added together.

For example, `uap->start = 0xffffffff` and `uap->len = 1` will overflow to 0 when added together, which will bypass the check.

Later on, there is a signed array index with a loop starting from `uap->start`. If `uap->start` is negative, this would index `iomap` negatively.

sys/amd64/amd64/sys_machdep:

int
amd64_set_ioperm(td, uap)
	struct thread *td;
	struct i386_ioperm_args *uap;
{
	int i, error;
	char *iomap;
	struct amd64tss *tssp;
	struct system_segment_descriptor *tss_sd;
	struct pcb *pcb;

	if ((error = priv_check(td, PRIV_IO)) != 0)
		return (error);
	if ((error = securelevel_gt(td->td_ucred, 0)) != 0)
		return (error);
	if (uap->start + uap->length > IOPAGES * PAGE_SIZE * NBBY)
		return (EINVAL);

	...

	for (i = uap->start; i < uap->start + uap->length; i++) {
		if (uap->enable)
			iomap[i >> 3] &= ~(1 << (i & 7));
		else
			iomap[i >> 3] |= (1 << (i & 7));
	}
	return (error);
}
Comment 1 commit-hook freebsd_committer 2016-05-20 15:33:05 UTC
A commit references this bug:

Author: kib
Date: Fri May 20 15:32:48 UTC 2016
New revision: 300305
URL: https://svnweb.freebsd.org/changeset/base/300305

Log:
  Use unsigned type for the loop index to make overflow checks effective.

  PR:	209661
  Reported by:	cturt
  Sponsored by:	The FreeBSD Foundation
  MFC after:	3 days

Changes:
  head/sys/amd64/amd64/sys_machdep.c
Comment 2 CTurt 2016-05-20 17:53:53 UTC
Sorry, I made a mistake in my report; the bound check is incorrect, as described, however, no negative array index will occur from this since an unsigned comparison is used in the loop. The impact is simply that `0` will be returned instead of `EINVAL`.

This patch is good because unsigned is more appropriate for `i`, however it doesn't fix the bug.

An additional patch should be applied to validate user arguments more effectively than the current bound check, so that the arguments I posted originally will no longer be accepted.
Comment 3 Konstantin Belousov freebsd_committer 2016-05-20 18:53:06 UTC
(In reply to CTurt from comment #2)
Suppose that uap->start is > 0x80000000, and uap->length is large enough so that the sum appears under the 3 pages * 8.  Then negative i falls under the loop condition and causes wrong accesses.

WRT return of success in this case, right, thank you for noting, I will fix it as well.
Comment 4 CTurt 2016-05-20 19:20:15 UTC
That negative loop was indeed my original theory I was trying to explain, however I no longer think that this is true. In the loop `i` is compared against an unsigned value, so an unsigned comparison will be used. That is to say, a negative value of `i` is counted as a large positive value for the comparison.

The result of this is that the loop condition won't be satisfied, and the array won't be indexed.
Comment 5 Konstantin Belousov freebsd_committer 2016-05-20 19:31:54 UTC
(In reply to Konstantin Belousov from comment #3)
Hm, ok.  Still I think that the change of i to unsigned is for better.

And, don't we have the same issue on i386 ?
Comment 6 Konstantin Belousov freebsd_committer 2016-05-20 19:33:42 UTC
Created attachment 170517 [details]
amd64 + i386 overflow check
Comment 7 CTurt 2016-05-20 19:39:21 UTC
Yes, as I said before, the change for `i` to unsigned is also good.

This latest patch fixes all my concerns.
Comment 8 commit-hook freebsd_committer 2016-05-20 19:50:38 UTC
A commit references this bug:

Author: kib
Date: Fri May 20 19:50:33 UTC 2016
New revision: 300332
URL: https://svnweb.freebsd.org/changeset/base/300332

Log:
  Check for overflow and return EINVAL if detected.  Backport this and
  r300305 to i386.

  PR:	209661
  Reported and reviewed by:	cturt
  Sponsored by:	The FreeBSD Foundation
  MFC after:	3 days

Changes:
  head/sys/amd64/amd64/sys_machdep.c
  head/sys/i386/i386/sys_machdep.c