| Message ID | 701fc3ee1136bbb4a0fd3d560c3ec1ee3bb3b828.1386798254.git.ydroneaud@opteya.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
On Wed, Dec 11, 2013 at 2:01 PM, Yann Droneaud <ydroneaud@opteya.com> wrote: > Just like vfs_read(), uverbs_write() must check output buffer > (eg. response) with access_ok(VERIFY_WRITE,...) to ensure > it's in userspace memory before using the pointer in uverbs > functions. Is there any place where we use this pointer through something other than copy_to_user()? I don't think there is or ever should be, in which case this check is redundant. - R. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Roland, > On Wed, Dec 11, 2013 at 2:01 PM, Yann Droneaud <ydroneaud@opteya.com> wrote: >> Just like vfs_read(), uverbs_write() must check output buffer >> (eg. response) with access_ok(VERIFY_WRITE,...) to ensure >> it's in userspace memory before using the pointer in uverbs >> functions. > Is there any place where we use this pointer through something other > than copy_to_user()? I don't think there is or ever should be, in > which case this check is redundant. Since this check is applied on the 'extended' uverbs, there's only a limited amount of existing code to check. Anyway the purpose of this check is to ensure that *whole* buffer passed as response buffer is valid, and the whole buffer may be larger than the response to be returned. Just like the check in read(2) syscall, it's a sanity check to refuse to process malformed syscall: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/fs/read_write.c#n389 This particular check was added by Linus in 2005, the commit message might be interesting: https://git.kernel.org/cgit/linux/kernel/git/tglx/history.git/commit/?id=fd770e66c9a65b14ce114e171266cf6f393df502 Make read/write always do the full "access_ok()" tests. The actual user copy will do them too, but only for the range that ends up being actually copied. That hides bugs when the range has been clamped by file size or other issues. So I don't see this check as redundant with call to copy_to_user(), it's checking a different thing. Regards.
diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c index 8652c13f6ea2..0be1dd86f768 100644 --- a/drivers/infiniband/core/uverbs_main.c +++ b/drivers/infiniband/core/uverbs_main.c @@ -677,6 +677,11 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, if (response) { if (!hdr.out_words && !ex_hdr.provider_out_words) return -EINVAL; + + if (!access_ok(VERIFY_WRITE, + response, + (hdr.out_words + ex_hdr.provider_out_words) * 8)) + return -EFAULT; } else { if (hdr.out_words || ex_hdr.provider_out_words) return -EINVAL;
Just like vfs_read(), uverbs_write() must check output buffer (eg. response) with access_ok(VERIFY_WRITE,...) to ensure it's in userspace memory before using the pointer in uverbs functions. If the buffer or a subset of the buffer is not valid, returns -EFAULT. Note: there's no need to check input buffer (eg. command) since vfs_write() does the check access_ok(VERIFY_READ, ...) as part of write() syscall. Link: http://marc.info/?i=cover.1386798254.git.ydroneaud@opteya.com> Signed-off-by: Yann Droneaud <ydroneaud@opteya.com> --- drivers/infiniband/core/uverbs_main.c | 5 +++++ 1 file changed, 5 insertions(+)