create_flow/destroy_flow fixes for v3.13
v3 cover create_flow/destroy_flow fixes for v3.13
Hi Roland,
Please find the third revision of a patchset against create_flow/destroy_flow and associated extended command scheme.
These are fixes that *must* be applied before making the new uverbs widely available in v3.13:
- patches 1 and 2 address a warning reported by sparse.
- patch 3 handle an uncommon type of extended command.
- patch 4, 5 and 6 ensure that commands will be extensible:
- one patch add a missing check of comp_mask;
- the other patches add checks on reserved fields.
- patch 7 fix an error path which was not setting the error code.
- patch 8 ensure the response buffer of an extended command is a valid memory region for userspace. Doing the check earlier, on the *full* response buffer, might be safer.
- patch 9 prevent out of bound access and underflow.
Please review and apply for v3.13.
Changes from patchset v2 [1]:
- add a patch to check the input length in extended uverbs, create_flow and destroy_flow.
Changes from patchset v1 [2]:
- add patch to verify early response buffer using access_ok(), just like vfs_read().
Regards.
[1] [PATCH v2 for v3.13 0/8] create_flow/destroy_flow fixes for v3.13
http://marc.info/?i=cover.1385981934.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
[2] [PATCH for v3.13 0/7] create_flow/destroy_flow fixes for v3.13
http://marc.info/?i=cover.1385501822.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Yann Droneaud (9):
IB/core: const'ify inbuf in struct ib_udata
IB/uverbs: remove implicit cast in INIT_UDATA()
IB/uverbs: set outbuf to NULL when no core response space is provided IB/uverbs: check reserved field in extended command header IB/uverbs: check comp_mask in destroy_flow
IB/uverbs: check reserved fields in create_flow
IB/uverbs: set error code when fail to consume all flow_spec items IB/uverbs: check access to userspace response buffer in extended
command
IB/uverbs: check input length in flow steering uverbs
drivers/infiniband/core/uverbs.h | 12 ++++++------ drivers/infiniband/core/uverbs_cmd.c | 37 +++++++++++++++++++++++++---------- drivers/infiniband/core/uverbs_main.c | 21 +++++++++++++++----- include/rdma/ib_verbs.h | 2 +- 4 files changed, 50 insertions(+), 22 deletions(-)
Signature · 6 lines
1.8.4.2 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): cover.1386798254.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
PATCH v3 1/9 IB/core: const'ify inbuf in struct ib_udata
Userspace input buffer is not modified by kernel,
let it be 'const'.
It's also a prerequisite to remove the implicit cast from INIT_UDATA().
--- drivers/infiniband/core/uverbs.h | 2 +- include/rdma/ib_verbs.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
drivers/infiniband/core/uverbs.h +1 -1
@@ -49,7 +49,7 @@#define INIT_UDATA (udata, ibuf, obuf, ilen, olen) \ do { \ - (udata)->inbuf = (void __user *) (ibuf); \ + (udata)->inbuf = (const void __user *) (ibuf); \ (udata)->outbuf = (void __user *) (obuf); \ (udata)->inlen = (ilen); \ (udata)->outlen = (olen); \
include/rdma/ib_verbs.h +1 -1
@@ -978,7 +978,7 @@ struct ib_uobject { };struct ib_udata { -void __user *inbuf; +const void __user *inbuf;void __user *outbuf;size_t inlen;size_t outlen;
Signature · 6 lines
1.8.4.2 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v3 2/9 IB/uverbs: remove implicit cast in INIT_UDATA()
In reply to a message not in the archiveCurrently, INIT_UDATA() does an implicit cast to a pointer, so that 'response' address, eg. output buffer, can be used as is to initialize a struct ib_udata:
do { \
(udata)->inbuf = (void __user *) (ibuf); \
(udata)->outbuf = (void __user *) (obuf); \
(udata)->inlen = (ilen); \
(udata)->outlen = (olen); \
} while (0)...
INIT_UDATA(&udata, buf + sizeof cmd,
(unsigned long) cmd.response + sizeof resp, in_len - sizeof cmd, out_len - sizeof resp);
...
Hidding the integer to pointer conversion is prone to error that won't be catched by compiler/static analyzer is some case.
In the other hand, sparse reports an error if literal 0 is used to initialize inbuf or outbuf, for example in:
INIT_UDATA(&ucore,
(hdr.in_words) ? buf : 0,
(unsigned long)ex_hdr.response,
hdr.in_words * 8,
hdr.out_words * 8);It was reported by kbuild test robot in message[1]:
From: kbuild test robot <[email protected]>
Subject: "drivers/infiniband/core/uverbs_main.c:683:17:
sparse: Using plain integer as NULL pointer",
Message-Id: <528b3984.SVGs20ZWpcuR/Jls%[email protected]>
This patch fixes the warnings reported by sparse and allows the compiler to report a warning in case a plain integer get used to initialize a udata pointer.
This patch requires struct ib_udata to be modified to have a const void __user *inbuf field[2], otherwise compiler will report warnings regarding const to non const conversion:
drivers/infiniband/core/uverbs_main.c: In function ‘ib_uverbs_write’:drivers/infiniband/core/uverbs_main.c:682:24: attention : assignment discards ‘const’ qualifier from pointer target type [enabled by default] drivers/infiniband/core/uverbs_main.c:688:22: attention : assignment discards ‘const’ qualifier from pointer target type [enabled by default] drivers/infiniband/core/uverbs_cmd.c: In function ‘ib_uverbs_get_context’:drivers/infiniband/core/uverbs_cmd.c:307:23: attention : assignment discards ‘const’ qualifier from pointer target type [enabled by default] drivers/infiniband/core/uverbs_cmd.c: In function ‘ib_uverbs_alloc_pd’:drivers/infiniband/core/uverbs_cmd.c:516:23: attention : assignment discards ‘const’ qualifier from pointer target type [enabled by default]
...
[1] https://lists.01.org/pipermail/kbuild-all/2013-November/002120.html
[2] https://patchwork.kernel.org/patch/2846202/
http://marc.info/?i=3050a98379b4342ea59d59aeaf1ce162171df928.1376847403.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
--- drivers/infiniband/core/uverbs.h | 12 ++++++------ drivers/infiniband/core/uverbs_cmd.c | 20 ++++++++++---------- drivers/infiniband/core/uverbs_main.c | 13 ++++++++----- 3 files changed, 24 insertions(+), 21 deletions(-)
drivers/infiniband/core/uverbs.h +6 -6
@@ -47,12 +47,12 @@#include <rdma/ib_umem.h> #include <rdma/ib_user_verbs.h> -#define INIT_UDATA (udata, ibuf, obuf, ilen, olen) \ - do { \ - (udata)->inbuf = (const void __user *) (ibuf); \ - (udata)->outbuf = (void __user *) (obuf); \ - (udata)->inlen = (ilen); \ - (udata)->outlen = (olen); \ +#define INIT_UDATA (udata, ibuf, obuf, ilen, olen) \ + do { \ + (udata)->inbuf = (ibuf); \ + (udata)->outbuf = (obuf); \ + (udata)->inlen = (ilen); \ + (udata)->outlen = (olen); \ } while (0)/*
drivers/infiniband/core/uverbs_cmd.c +10 -10
@@ -305,7 +305,7 @@ ssize_t ib_uverbs_get_context(struct ib_uverbs_file *file, }INIT_UDATA (&udata, buf +sizeof cmd, - (unsigned long ) cmd.response +sizeof resp, + (void __user *)(unsigned long )cmd.response +sizeof resp, in_len -sizeof cmd, out_len -sizeof resp); ucontext = ibdev->alloc_ucontext (ibdev, &udata);
@@ -514,7 +514,7 @@ ssize_t ib_uverbs_alloc_pd(struct ib_uverbs_file *file,return -EFAULT ;INIT_UDATA (&udata, buf +sizeof cmd, - (unsigned long ) cmd.response +sizeof resp, + (void __user *)(unsigned long )cmd.response +sizeof resp, in_len -sizeof cmd, out_len -sizeof resp); uobj =kmalloc (sizeof *uobj,GFP_KERNEL );
@@ -711,7 +711,7 @@ ssize_t ib_uverbs_open_xrcd(struct ib_uverbs_file *file,return -EFAULT ;INIT_UDATA (&udata, buf +sizeof cmd, - (unsigned long ) cmd.response +sizeof resp, + (void __user *)(unsigned long )cmd.response +sizeof resp, in_len -sizeof cmd, out_len -sizeof resp);mutex_lock (&file->device->xrcd_tree_mutex);
@@ -923,7 +923,7 @@ ssize_t ib_uverbs_reg_mr(struct ib_uverbs_file *file,return -EFAULT ;INIT_UDATA (&udata, buf +sizeof cmd, - (unsigned long ) cmd.response +sizeof resp, + (void __user *)(unsigned long )cmd.response +sizeof resp, in_len -sizeof cmd, out_len -sizeof resp);if ((cmd.start & ~PAGE_MASK ) != (cmd.hca_va & ~PAGE_MASK ))
@@ -1215,7 +1215,7 @@ ssize_t ib_uverbs_create_cq(struct ib_uverbs_file *file,return -EFAULT ;INIT_UDATA (&udata, buf +sizeof cmd, - (unsigned long ) cmd.response +sizeof resp, + (void __user *)(unsigned long )cmd.response +sizeof resp, in_len -sizeof cmd, out_len -sizeof resp);if (cmd.comp_vector >= file->device->num_comp_vectors)
@@ -1311,7 +1311,7 @@ ssize_t ib_uverbs_resize_cq(struct ib_uverbs_file *file,return -EFAULT ;INIT_UDATA (&udata, buf +sizeof cmd, - (unsigned long ) cmd.response +sizeof resp, + (void __user *)(unsigned long )cmd.response +sizeof resp, in_len -sizeof cmd, out_len -sizeof resp); cq =idr_read_cq (cmd.cq_handle, file->ucontext,0 );
@@ -1513,7 +1513,7 @@ ssize_t ib_uverbs_create_qp(struct ib_uverbs_file *file,return -EPERM ;INIT_UDATA (&udata, buf +sizeof cmd, - (unsigned long ) cmd.response +sizeof resp, + (void __user *)(unsigned long )cmd.response +sizeof resp, in_len -sizeof cmd, out_len -sizeof resp); obj =kzalloc (sizeof *obj,GFP_KERNEL );
@@ -1700,7 +1700,7 @@ ssize_t ib_uverbs_open_qp(struct ib_uverbs_file *file,return -EFAULT ;INIT_UDATA (&udata, buf +sizeof cmd, - (unsigned long ) cmd.response +sizeof resp, + (void __user *)(unsigned long )cmd.response +sizeof resp, in_len -sizeof cmd, out_len -sizeof resp); obj =kmalloc (sizeof *obj,GFP_KERNEL );
@@ -2976,7 +2976,7 @@ ssize_t ib_uverbs_create_srq(struct ib_uverbs_file *file, xcmd.srq_limit = cmd.srq_limit;INIT_UDATA (&udata, buf +sizeof cmd, - (unsigned long ) cmd.response +sizeof resp, + (void __user *)(unsigned long )cmd.response +sizeof resp, in_len -sizeof cmd, out_len -sizeof resp); ret =__uverbs_create_xsrq (file, &xcmd, &udata);
@@ -3001,7 +3001,7 @@ ssize_t ib_uverbs_create_xsrq(struct ib_uverbs_file *file,return -EFAULT ;INIT_UDATA (&udata, buf +sizeof cmd, - (unsigned long ) cmd.response +sizeof resp, + (void __user *)(unsigned long )cmd.response +sizeof resp, in_len -sizeof cmd, out_len -sizeof resp); ret =__uverbs_create_xsrq (file, &cmd, &udata);
drivers/infiniband/core/uverbs_main.c +8 -5
@@ -635,6 +635,7 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,__u32 command;struct ib_uverbs_ex_cmd_hdr ex_hdr; +char __user *response;struct ib_udata ucore;struct ib_udata uhw;int err;
@@ -668,7 +669,9 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,if ((hdr.in_words + ex_hdr.provider_in_words) *8 != count)return -EINVAL ; -if (ex_hdr.response) { + response = (char __user *)(unsigned long )ex_hdr.response; + +if (response) {if (!hdr.out_words && !ex_hdr.provider_out_words)return -EINVAL ; }else {
@@ -677,14 +680,14 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, }INIT_UDATA (&ucore, - (hdr.in_words) ? buf :0 , - (unsigned long )ex_hdr.response, + (hdr.in_words) ? buf :NULL , + response, hdr.in_words *8 , hdr.out_words *8 );INIT_UDATA (&uhw, - (ex_hdr.provider_in_words) ? buf + ucore.inlen :0 , - (ex_hdr.provider_out_words) ? (unsigned long )ex_hdr.response + ucore.outlen :0 , + (ex_hdr.provider_in_words) ? buf + ucore.inlen :NULL , + (ex_hdr.provider_out_words) ? response + ucore.outlen :NULL , ex_hdr.provider_in_words *8 , ex_hdr.provider_out_words *8 );
Signature · 6 lines
1.8.4.2 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v3 3/9 IB/uverbs: set outbuf to NULL when no core response space is provided
In reply to a message not in the archiveIn the currently uncommon case of core (eg. uverbs) response space being omitted, but hw (eg. provider) response space being available, outbuf get defined to "response" while it must be NULL.
This patch takes care of setting ucore->outbuf to NULL if hdr.out_words is equal to 0.
--- drivers/infiniband/core/uverbs_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/infiniband/core/uverbs_main.c +1 -1
@@ -681,7 +681,7 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,INIT_UDATA (&ucore, (hdr.in_words) ? buf :NULL , - response, + (hdr.out_words) ? response :NULL , hdr.in_words *8 , hdr.out_words *8 );
Signature · 6 lines
1.8.4.2 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v3 4/9 IB/uverbs: check reserved field in extended command header
In reply to a message not in the archiveAs noted by Daniel Vetter in its article "Botching up ioctls"[1]
"Check *all* unused fields and flags and all the padding for
whether it's 0, and reject the ioctl if that's not the case. Otherwise your nice plan for future extensions is going right down the gutters since someone *will* submit an ioctl struct with random stack garbage in the yet unused parts. Which then bakes in the ABI that those fields can never be used for anything else but garbage."
It's important to ensure that reserved fields are set to known value, so that it will be possible to use them latter to extend the ABI.
The same reasonning apply to comp_mask field present in newer uverbs command: per commit 22878dbc9173, unsupported values in comp_mask are rejected.
[1] http://blog.ffwll.ch/2013/11/botching-up-ioctls.html
--- drivers/infiniband/core/uverbs_main.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/infiniband/core/uverbs_main.c +3 -0
@@ -669,6 +669,9 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,if ((hdr.in_words + ex_hdr.provider_in_words) *8 != count)return -EINVAL ; +if (ex_hdr.cmd_hdr_reserved) +return -EINVAL ; + response = (char __user *)(unsigned long )ex_hdr.response;if (response) {
Signature · 6 lines
1.8.4.2 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v3 5/9 IB/uverbs: check comp_mask in destroy_flow
In reply to a message not in the archiveJust like the check added to create_flow in 22878dbc9173, comp_mask must be checked in destroy_flow too.
Since only empty comp_mask is currently supported, any other value must be rejected.
This check was silently added in a previous patch[1] to move comp_mask in extended command header, part of previous patchset[2] against create/destroy_flow uverbs. The idea of moving comp_mask to the header was discarded for the final patchset[3].
Unfortunately the check added in destroy_flow uverb was not integrated in the final patchset.
[1] http://marc.info/?i=40175eda10d670d098204da6aa4c327a0171ae5f.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
[2] http://marc.info/?i=cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
[3] http://marc.info/?i=cover.1383773832.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
--- drivers/infiniband/core/uverbs_cmd.c | 3 +++ 1 file changed, 3 insertions(+)
drivers/infiniband/core/uverbs_cmd.c +3 -0
@@ -2795,6 +2795,9 @@ int ib_uverbs_ex_destroy_flow(struct ib_uverbs_file *file,if (ret)return ret; +if (cmd.comp_mask) +return -EINVAL ; + uobj =idr_write_uobj (&ib_uverbs_rule_idr, cmd.flow_handle, file->ucontext);if (!uobj)
Signature · 6 lines
1.8.4.2 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v3 6/9 IB/uverbs: check reserved fields in create_flow
In reply to a message not in the archiveAs noted by Daniel Vetter in its article "Botching up ioctls"[1]
"Check *all* unused fields and flags and all the padding for
whether it's 0, and reject the ioctl if that's not the case. Otherwise your nice plan for future extensions is going right down the gutters since someone *will* submit an ioctl struct with random stack garbage in the yet unused parts. Which then bakes in the ABI that those fields can never be used for anything else but garbage."
It's important to ensure that reserved fields are set to known value, so that it will be possible to use them latter to extend the ABI.
The same reasonning apply to comp_mask field present in newer uverbs command: per commit 22878dbc9173, unsupported values in comp_mask are rejected.
[1] http://blog.ffwll.ch/2013/11/botching-up-ioctls.html
--- drivers/infiniband/core/uverbs_cmd.c | 7 +++++++ 1 file changed, 7 insertions(+)
drivers/infiniband/core/uverbs_cmd.c +7 -0
@@ -2593,6 +2593,9 @@ out_put:static int kern_spec_to_ib_spec (struct ib_uverbs_flow_spec *kern_spec,union ib_flow_spec *ib_spec) { +if (kern_spec->reserved) +return -EINVAL ; + ib_spec->type = kern_spec->type;switch (ib_spec->type) {
@@ -2671,6 +2674,10 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file, (cmd.flow_attr.num_of_specs * sizeof(struct ib_uverbs_flow_spec)))return -EINVAL ; +if (cmd.flow_attr.reserved[0 ] || + cmd.flow_attr.reserved[1 ]) +return -EINVAL ; +if (cmd.flow_attr.num_of_specs) { kern_flow_attr =kmalloc (sizeof (*kern_flow_attr) + cmd.flow_attr.size,GFP_KERNEL );
Signature · 6 lines
1.8.4.2 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v3 7/9 IB/uverbs: set error code when fail to consume all flow_spec items
In reply to a message not in the archiveIf the flow_spec items parsed count does not match the number of items declared in the flow_attr command, or if not all bytes are used for flow_spec items (eg. trailing garbage), a log message is reported and the function leave through the error path. Unfortunately the error code is currently not set.
This patch set error code to -EINVAL in such cases, so that the error is reported to userspace instead of silently fail.
--- drivers/infiniband/core/uverbs_cmd.c | 1 + 1 file changed, 1 insertion(+)
drivers/infiniband/core/uverbs_cmd.c +1 -0
@@ -2738,6 +2738,7 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,if (cmd.flow_attr.size || (i != flow_attr->num_of_specs)) {pr_warn ("create flow failed, flow %d: %d bytes left from uverb cmd\n" , i, cmd.flow_attr.size); + err = -EINVAL ; goto err_free; } flow_id =ib_create_flow (qp, flow_attr,IB_FLOW_DOMAIN_USER );
Signature · 6 lines
1.8.4.2 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v3 8/9 IB/uverbs: check access to userspace response buffer in extended command
In reply to a message not in the archiveJust 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.
--- drivers/infiniband/core/uverbs_main.c | 5 +++++ 1 file changed, 5 insertions(+)
drivers/infiniband/core/uverbs_main.c +5 -0
@@ -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 ;
Signature · 6 lines
1.8.4.2 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 701fc3ee1136bbb4a0fd3d560c3ec1ee3bb3b828.1386798254.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Re: [PATCHv3 for-3.13 8/9] IB/uverbs: check access to userspace response buffer in extended command
Quoted 4 lines · Yann Droneaud
Just like
vfs_read(),uverbs_write()must check output buffer (eg. response) withaccess_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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): CAL1RGDVThe_=F3EOCzYfxB8zCgv+4Z_PXiYEPqnphMEOi-drbw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org
Re: [PATCHv3 for-3.13 8/9] IB/uverbs: check access to userspace response buffer in extended command
Hi Roland,
Quoted 9 lines · Yann Droneaud
Quoted 4 lines
Just like
vfs_read(),uverbs_write()must check output buffer (eg. response) withaccess_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:
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.
Signature · 6 lines
Yann Droneaud OPTEYA -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v3 9/9 IB/uverbs: check input length in flow steering uverbs
In reply to a message not in the archiveSince ib_copy_from_udata() doesn't check yet the available input data length before accessing userspace memory, an explicit check of this length is required to prevent:
- reading past the user provided buffer,
- underflow when subtracting the expected command size from the input length.
This will ensure the newly added flow steering uverbs don't try to process truncated commands.
--- drivers/infiniband/core/uverbs_cmd.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/infiniband/core/uverbs_cmd.c +6 -0
@@ -2649,6 +2649,9 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,void *ib_spec;int i; +if (ucore->inlen <sizeof (cmd)) +return -EINVAL ; +if (ucore->outlen <sizeof (resp))return -ENOSPC ;
@@ -2799,6 +2802,9 @@ int ib_uverbs_ex_destroy_flow(struct ib_uverbs_file *file,struct ib_uobject *uobj;int ret; +if (ucore->inlen <sizeof (cmd)) +return -EINVAL ; + ret =ib_copy_from_udata (&cmd, ucore,sizeof (cmd));if (ret)return ret;
Signature · 6 lines
1.8.4.2 -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html