Commit 8aec0f5d4137532de14e6554fd5dd201ff3a3c49
Committed by
Linus Torvalds
1 parent
c39ac49f23
Exists in
master
and in
20 other branches
Fix: compat_rw_copy_check_uvector() misuse in aio, readv, writev, and security keys
Looking at mm/process_vm_access.c:process_vm_rw() and comparing it to compat_process_vm_rw() shows that the compatibility code requires an explicit "access_ok()" check before calling compat_rw_copy_check_uvector(). The same difference seems to appear when we compare fs/read_write.c:do_readv_writev() to fs/compat.c:compat_do_readv_writev(). This subtle difference between the compat and non-compat requirements should probably be debated, as it seems to be error-prone. In fact, there are two others sites that use this function in the Linux kernel, and they both seem to get it wrong: Now shifting our attention to fs/aio.c, we see that aio_setup_iocb() also ends up calling compat_rw_copy_check_uvector() through aio_setup_vectored_rw(). Unfortunately, the access_ok() check appears to be missing. Same situation for security/keys/compat.c:compat_keyctl_instantiate_key_iov(). I propose that we add the access_ok() check directly into compat_rw_copy_check_uvector(), so callers don't have to worry about it, and it therefore makes the compat call code similar to its non-compat counterpart. Place the access_ok() check in the same location where copy_from_user() can trigger a -EFAULT error in the non-compat code, so the ABI behaviors are alike on both compat and non-compat. While we are here, fix compat_do_readv_writev() so it checks for compat_rw_copy_check_uvector() negative return values. And also, fix a memory leak in compat_keyctl_instantiate_key_iov() error handling. Acked-by: Linus Torvalds <torvalds@linux-foundation.org> Acked-by: Al Viro <viro@ZenIV.linux.org.uk> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Showing 3 changed files with 9 additions and 18 deletions Side-by-side Diff
fs/compat.c
... | ... | @@ -558,6 +558,10 @@ |
558 | 558 | } |
559 | 559 | *ret_pointer = iov; |
560 | 560 | |
561 | + ret = -EFAULT; | |
562 | + if (!access_ok(VERIFY_READ, uvector, nr_segs*sizeof(*uvector))) | |
563 | + goto out; | |
564 | + | |
561 | 565 | /* |
562 | 566 | * Single unix specification: |
563 | 567 | * We should -EINVAL if an element length is not >= 0 and fitting an |
564 | 568 | |
565 | 569 | |
566 | 570 | |
... | ... | @@ -1080,17 +1084,12 @@ |
1080 | 1084 | if (!file->f_op) |
1081 | 1085 | goto out; |
1082 | 1086 | |
1083 | - ret = -EFAULT; | |
1084 | - if (!access_ok(VERIFY_READ, uvector, nr_segs*sizeof(*uvector))) | |
1085 | - goto out; | |
1086 | - | |
1087 | - tot_len = compat_rw_copy_check_uvector(type, uvector, nr_segs, | |
1087 | + ret = compat_rw_copy_check_uvector(type, uvector, nr_segs, | |
1088 | 1088 | UIO_FASTIOV, iovstack, &iov); |
1089 | - if (tot_len == 0) { | |
1090 | - ret = 0; | |
1089 | + if (ret <= 0) | |
1091 | 1090 | goto out; |
1092 | - } | |
1093 | 1091 | |
1092 | + tot_len = ret; | |
1094 | 1093 | ret = rw_verify_area(type, file, pos, tot_len); |
1095 | 1094 | if (ret < 0) |
1096 | 1095 | goto out; |
mm/process_vm_access.c
... | ... | @@ -429,12 +429,6 @@ |
429 | 429 | if (flags != 0) |
430 | 430 | return -EINVAL; |
431 | 431 | |
432 | - if (!access_ok(VERIFY_READ, lvec, liovcnt * sizeof(*lvec))) | |
433 | - goto out; | |
434 | - | |
435 | - if (!access_ok(VERIFY_READ, rvec, riovcnt * sizeof(*rvec))) | |
436 | - goto out; | |
437 | - | |
438 | 432 | if (vm_write) |
439 | 433 | rc = compat_rw_copy_check_uvector(WRITE, lvec, liovcnt, |
440 | 434 | UIO_FASTIOV, iovstack_l, |
... | ... | @@ -459,8 +453,6 @@ |
459 | 453 | kfree(iov_r); |
460 | 454 | if (iov_l != iovstack_l) |
461 | 455 | kfree(iov_l); |
462 | - | |
463 | -out: | |
464 | 456 | return rc; |
465 | 457 | } |
466 | 458 |
security/keys/compat.c
... | ... | @@ -40,12 +40,12 @@ |
40 | 40 | ARRAY_SIZE(iovstack), |
41 | 41 | iovstack, &iov); |
42 | 42 | if (ret < 0) |
43 | - return ret; | |
43 | + goto err; | |
44 | 44 | if (ret == 0) |
45 | 45 | goto no_payload_free; |
46 | 46 | |
47 | 47 | ret = keyctl_instantiate_key_common(id, iov, ioc, ret, ringid); |
48 | - | |
48 | +err: | |
49 | 49 | if (iov != iovstack) |
50 | 50 | kfree(iov); |
51 | 51 | return ret; |