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); }
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
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.
(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.
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.
(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 ?
Created attachment 170517 [details] amd64 + i386 overflow check
Yes, as I said before, the change for `i` to unsigned is also good. This latest patch fixes all my concerns.
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