| Message ID | 20200114081455.1240-1-kamalheib1@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [for-next,V2] RDMA/core: Fix storing node description | expand |
On Tue, Jan 14, 2020 at 10:14:55AM +0200, Kamal Heib wrote: > Make sure to return -EINVAL when the supplied string is bigger then > the node_desc array. > > Fixes: c5bcbbb9fe00 ("IB: Allow userspace to set node description") > Signed-off-by: Kamal Heib <kamalheib1@gmail.com> > --- > drivers/infiniband/core/sysfs.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c > index 087682e6969e..aa90a42d6565 100644 > --- a/drivers/infiniband/core/sysfs.c > +++ b/drivers/infiniband/core/sysfs.c > @@ -1268,7 +1268,9 @@ static ssize_t node_desc_store(struct device *device, > if (!dev->ops.modify_device) > return -EOPNOTSUPP; > > - memcpy(desc.node_desc, buf, min_t(int, count, IB_DEVICE_NODE_DESC_MAX)); > + if (strscpy(desc.node_desc, buf, sizeof(desc.node_desc)) == -E2BIG) > + return -EINVAL; > + So, while this is the preferred code, when I checked on how this all works what I found was madness. The desc.node_desc is not a string. It is an array of 64 bytes, where all 64 bits can be valid without a null termination. Code that accesses the array like: drivers/infiniband/core/sysfs.c: return sprintf(buf, "%.64s\n", dev->node_desc); Is 'correct', while code like this: drivers/infiniband/hw/bnxt_re/main.c: return scnprintf(buf, PAGE_SIZE, "%s\n", rdev->ibdev.node_desc); Is wrong, it could run past the end of the character array. Obviously this is all insane. If I apply your patch then userspace looses the ability to have a 64 character node description. If you want to persue this then please fix the underlying issue - make node_desc into an always null terminated string: - Increase the width to 65 - Never use memcpy to manipulate it - Devices should set it from the mad/device array using scnprintf(dev->node_desc, sizeof(dev->node_desc), "%.64s\n", mad_desc) Perhaps this should be a helper function - Devices should read it into a MAD format using some approach zero fills - Get rid of all the .64 format strings Thanks, Jason
diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c index 087682e6969e..aa90a42d6565 100644 --- a/drivers/infiniband/core/sysfs.c +++ b/drivers/infiniband/core/sysfs.c @@ -1268,7 +1268,9 @@ static ssize_t node_desc_store(struct device *device, if (!dev->ops.modify_device) return -EOPNOTSUPP; - memcpy(desc.node_desc, buf, min_t(int, count, IB_DEVICE_NODE_DESC_MAX)); + if (strscpy(desc.node_desc, buf, sizeof(desc.node_desc)) == -E2BIG) + return -EINVAL; + ret = ib_modify_device(dev, IB_DEVICE_MODIFY_NODE_DESC, &desc); if (ret) return ret;
Make sure to return -EINVAL when the supplied string is bigger then the node_desc array. Fixes: c5bcbbb9fe00 ("IB: Allow userspace to set node description") Signed-off-by: Kamal Heib <kamalheib1@gmail.com> --- drivers/infiniband/core/sysfs.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)