| Message ID | 20191223093943.17883-1-kamalheib1@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [for-next] RDMA/core: Fix storing node_desc | expand |
On Mon, Dec 23, 2019 at 11:39:43AM +0200, Kamal Heib wrote: > When writing to node_desc sysfs using echo a new line symbol will be > stored at the end of the string, avoid that by dropping the new line Why do we want to do this? AFAIK technically new line is valid in a node description. > symbol and also make sure to return -EINVAL when the supplied string is > bigger then IB_DEVICE_NODE_DESC_MAX. This makes sense though > diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c > index 087682e6969e..2de5f6710c0b 100644 > +++ b/drivers/infiniband/core/sysfs.c > @@ -1263,12 +1263,21 @@ static ssize_t node_desc_store(struct device *device, > { > struct ib_device *dev = rdma_device_to_ibdev(device); > struct ib_device_modify desc = {}; > + size_t len; > int ret; > > if (!dev->ops.modify_device) > return -EOPNOTSUPP; > > - memcpy(desc.node_desc, buf, min_t(int, count, IB_DEVICE_NODE_DESC_MAX)); > + if (count > IB_DEVICE_NODE_DESC_MAX) > + return -EINVAL; > + > + len = strlen(buf); Why strlen? The buf is count bytes long. > + if (buf[len - 1] == '\n') > + len--; And if it is zero bytes this buffer underflows > + strncpy(desc.node_desc, buf, len); What was the point of switching away from memcpy? > + desc.node_desc[len] = 0; > ret = ib_modify_device(dev, IB_DEVICE_MODIFY_NODE_DESC, &desc); > if (ret) > return ret; Jason
On Fri, Jan 03, 2020 at 07:12:12PM -0400, Jason Gunthorpe wrote: > On Mon, Dec 23, 2019 at 11:39:43AM +0200, Kamal Heib wrote: > > When writing to node_desc sysfs using echo a new line symbol will be > > stored at the end of the string, avoid that by dropping the new line > > Why do we want to do this? AFAIK technically new line is valid in a > node description. > Self-Nack, please drop this patch, I didn't do a good job here. Thanks, Kamal > > symbol and also make sure to return -EINVAL when the supplied string is > > bigger then IB_DEVICE_NODE_DESC_MAX. > > This makes sense though > > > diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c > > index 087682e6969e..2de5f6710c0b 100644 > > +++ b/drivers/infiniband/core/sysfs.c > > @@ -1263,12 +1263,21 @@ static ssize_t node_desc_store(struct device *device, > > { > > struct ib_device *dev = rdma_device_to_ibdev(device); > > struct ib_device_modify desc = {}; > > + size_t len; > > int ret; > > > > if (!dev->ops.modify_device) > > return -EOPNOTSUPP; > > > > - memcpy(desc.node_desc, buf, min_t(int, count, IB_DEVICE_NODE_DESC_MAX)); > > > + if (count > IB_DEVICE_NODE_DESC_MAX) > > + return -EINVAL; > > + > > + len = strlen(buf); > > Why strlen? The buf is count bytes long. > > > + if (buf[len - 1] == '\n') > > + len--; > > And if it is zero bytes this buffer underflows > > > + strncpy(desc.node_desc, buf, len); > > What was the point of switching away from memcpy? > > > + desc.node_desc[len] = 0; > > ret = ib_modify_device(dev, IB_DEVICE_MODIFY_NODE_DESC, &desc); > > if (ret) > > return ret; > > Jason
diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c index 087682e6969e..2de5f6710c0b 100644 --- a/drivers/infiniband/core/sysfs.c +++ b/drivers/infiniband/core/sysfs.c @@ -1263,12 +1263,21 @@ static ssize_t node_desc_store(struct device *device, { struct ib_device *dev = rdma_device_to_ibdev(device); struct ib_device_modify desc = {}; + size_t len; int ret; if (!dev->ops.modify_device) return -EOPNOTSUPP; - memcpy(desc.node_desc, buf, min_t(int, count, IB_DEVICE_NODE_DESC_MAX)); + if (count > IB_DEVICE_NODE_DESC_MAX) + return -EINVAL; + + len = strlen(buf); + if (buf[len - 1] == '\n') + len--; + + strncpy(desc.node_desc, buf, len); + desc.node_desc[len] = 0; ret = ib_modify_device(dev, IB_DEVICE_MODIFY_NODE_DESC, &desc); if (ret) return ret;
When writing to node_desc sysfs using echo a new line symbol will be stored at the end of the string, avoid that by dropping the new line symbol and also make sure to return -EINVAL when the supplied string is bigger then IB_DEVICE_NODE_DESC_MAX. Fixes: c5bcbbb9fe00 ("IB: Allow userspace to set node description") Signed-off-by: Kamal Heib <kamalheib1@gmail.com> --- drivers/infiniband/core/sysfs.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)