| Message ID | 5ef05339c1d799133076c24e616860a647e96148.1560957168.git.dledford@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | RDMA/netlink: cleanups | expand |
On Wed, Jun 19, 2019 at 11:16:12AM -0400, Doug Ledford wrote: > For all string attributes for which we don't currently accept the > element as input, we only use it as output, set the string length to > RDMA_NLDEV_ATTR_EMPTY_STRING which is defined as 1. That way we will > only accept a null string for that element. This will prevent someone > from writing a new input routine that uses the element without also > updating the policy to have a valid value. > > Also while there, make sure the existing entries that are valid have the > correct policy, if not, correct the policy. Remove unnecessary checks > for nla_strlcpy() overflow once the policy has been set correctly. > > Signed-off-by: Doug Ledford <dledford@redhat.com> > drivers/infiniband/core/nldev.c | 24 +++++++++++------------- > include/uapi/rdma/rdma_netlink.h | 1 + > 2 files changed, 12 insertions(+), 13 deletions(-) > > v0->v1: Remove all whitespace change noise from this patch, this patch > is now all functional changes > > diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c > index 6006d23d0410..291d13642fcf 100644 > +++ b/drivers/infiniband/core/nldev.c > @@ -49,29 +49,29 @@ static const struct nla_policy nldev_policy[RDMA_NLDEV_ATTR_MAX] = { > [RDMA_NLDEV_ATTR_CHARDEV] = { .type = NLA_U64 }, > [RDMA_NLDEV_ATTR_CHARDEV_ABI] = { .type = NLA_U64 }, > [RDMA_NLDEV_ATTR_CHARDEV_NAME] = { .type = NLA_NUL_STRING, > - .len = RDMA_NLDEV_ATTR_ENTRY_STRLEN }, > + .len = RDMA_NLDEV_ATTR_EMPTY_STRING }, > [RDMA_NLDEV_ATTR_CHARDEV_TYPE] = { .type = NLA_NUL_STRING, > - .len = 128 }, > + .len = IB_DEVICE_NAME_MAX }, > [RDMA_NLDEV_ATTR_DEV_INDEX] = { .type = NLA_U32 }, > [RDMA_NLDEV_ATTR_DEV_NAME] = { .type = NLA_NUL_STRING, > - .len = IB_DEVICE_NAME_MAX - 1}, > + .len = IB_DEVICE_NAME_MAX }, > [RDMA_NLDEV_ATTR_DEV_NODE_TYPE] = { .type = NLA_U8 }, > [RDMA_NLDEV_ATTR_DEV_PROTOCOL] = { .type = NLA_NUL_STRING, > - .len = RDMA_NLDEV_ATTR_ENTRY_STRLEN }, > + .len = RDMA_NLDEV_ATTR_EMPTY_STRING }, > [RDMA_NLDEV_ATTR_DRIVER] = { .type = NLA_NESTED }, > [RDMA_NLDEV_ATTR_DRIVER_ENTRY] = { .type = NLA_NESTED }, > [RDMA_NLDEV_ATTR_DRIVER_PRINT_TYPE] = { .type = NLA_U8 }, > [RDMA_NLDEV_ATTR_DRIVER_STRING] = { .type = NLA_NUL_STRING, > - .len = RDMA_NLDEV_ATTR_ENTRY_STRLEN }, > + .len = RDMA_NLDEV_ATTR_EMPTY_STRING }, > [RDMA_NLDEV_ATTR_DRIVER_S32] = { .type = NLA_S32 }, > [RDMA_NLDEV_ATTR_DRIVER_S64] = { .type = NLA_S64 }, > [RDMA_NLDEV_ATTR_DRIVER_U32] = { .type = NLA_U32 }, > [RDMA_NLDEV_ATTR_DRIVER_U64] = { .type = NLA_U64 }, > [RDMA_NLDEV_ATTR_FW_VERSION] = { .type = NLA_NUL_STRING, > - .len = IB_FW_VERSION_NAME_MAX - 1}, > + .len = RDMA_NLDEV_ATTR_EMPTY_STRING }, > [RDMA_NLDEV_ATTR_LID] = { .type = NLA_U32 }, > [RDMA_NLDEV_ATTR_LINK_TYPE] = { .type = NLA_NUL_STRING, > - .len = RDMA_NLDEV_ATTR_ENTRY_STRLEN }, > + .len = IFNAMSIZ }, > [RDMA_NLDEV_ATTR_LMC] = { .type = NLA_U8 }, > [RDMA_NLDEV_ATTR_NDEV_INDEX] = { .type = NLA_U32 }, > [RDMA_NLDEV_ATTR_NDEV_NAME] = { .type = NLA_NUL_STRING, > @@ -92,7 +92,7 @@ static const struct nla_policy nldev_policy[RDMA_NLDEV_ATTR_MAX] = { > .len = sizeof(struct __kernel_sockaddr_storage) }, > [RDMA_NLDEV_ATTR_RES_IOVA] = { .type = NLA_U64 }, > [RDMA_NLDEV_ATTR_RES_KERN_NAME] = { .type = NLA_NUL_STRING, > - .len = TASK_COMM_LEN }, > + .len = RDMA_NLDEV_ATTR_EMPTY_STRING }, > [RDMA_NLDEV_ATTR_RES_LKEY] = { .type = NLA_U32 }, > [RDMA_NLDEV_ATTR_RES_LOCAL_DMA_LKEY] = { .type = NLA_U32 }, > [RDMA_NLDEV_ATTR_RES_LQPN] = { .type = NLA_U32 }, > @@ -120,7 +120,7 @@ static const struct nla_policy nldev_policy[RDMA_NLDEV_ATTR_MAX] = { > [RDMA_NLDEV_ATTR_RES_SUMMARY_ENTRY] = { .type = NLA_NESTED }, > [RDMA_NLDEV_ATTR_RES_SUMMARY_ENTRY_CURR]= { .type = NLA_U64 }, > [RDMA_NLDEV_ATTR_RES_SUMMARY_ENTRY_NAME]= { .type = NLA_NUL_STRING, > - .len = 16 }, > + .len = RDMA_NLDEV_ATTR_EMPTY_STRING }, > [RDMA_NLDEV_ATTR_RES_TYPE] = { .type = NLA_U8 }, > [RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY]= { .type = NLA_U32 }, > [RDMA_NLDEV_ATTR_RES_USECNT] = { .type = NLA_U64 }, > @@ -1372,10 +1372,8 @@ static int nldev_get_chardev(struct sk_buff *skb, struct nlmsghdr *nlh, > extack); > if (err || !tb[RDMA_NLDEV_ATTR_CHARDEV_TYPE]) > return -EINVAL; > - > - if (nla_strlcpy(client_name, tb[RDMA_NLDEV_ATTR_CHARDEV_TYPE], > - sizeof(client_name)) >= sizeof(client_name)) > - return -EINVAL; > + nla_strlcpy(client_name, tb[RDMA_NLDEV_ATTR_CHARDEV_TYPE], > + sizeof(client_name)); This seems really frail, it should at least have a BUILD_BUG_ON(sizeof(client_name) == nldev_policy[RDMA_NLDEV_ATTR_CHARDEV_TYPE].len)); But I don't think that can compile. Are we sure we can't have a 0 length and just skip checking in policy? It seems like more danger than it is worth. > if (tb[RDMA_NLDEV_ATTR_DEV_INDEX]) { > index = nla_get_u32(tb[RDMA_NLDEV_ATTR_DEV_INDEX]); > diff --git a/include/uapi/rdma/rdma_netlink.h b/include/uapi/rdma/rdma_netlink.h > index b27c02185dcc..24ff4ffa30dd 100644 > +++ b/include/uapi/rdma/rdma_netlink.h > @@ -285,6 +285,7 @@ enum rdma_nldev_command { > }; > > enum { > + RDMA_NLDEV_ATTR_EMPTY_STRING = 1, > RDMA_NLDEV_ATTR_ENTRY_STRLEN = 16, The empty thing is just an internal implementation detail, should not leak into uapi Jason
On Wed, 2019-06-19 at 16:24 -0300, Jason Gunthorpe wrote: > > > + nla_strlcpy(client_name, tb[RDMA_NLDEV_ATTR_CHARDEV_TYPE], > > + sizeof(client_name)); > > This seems really frail, it should at least have a > > BUILD_BUG_ON(sizeof(client_name) == > nldev_policy[RDMA_NLDEV_ATTR_CHARDEV_TYPE].len)); > > But I don't think that can compile. > > Are we sure we can't have a 0 length and just skip checking in policy? > It seems like more danger than it is worth. nla_strlcpy takes a size parameter as arg3 and guarantees to put no more than arg3 bytes - 1 from the source into the dest and guarantees it's null terminated. The only difference between nla_strlcpy and normal strncpy is that nla_strlcpy zeros out any excess pad bytes that aren't used in the dest by the copy. If someone tries to pass in an oversized string, it doesn't matter. If someone modifes the function to change the size of client_name, our nla_strlcpy() is safe because our dest and our len parameters are always in sync. I don't see the fragility. > > if (tb[RDMA_NLDEV_ATTR_DEV_INDEX]) { > > index = nla_get_u32(tb[RDMA_NLDEV_ATTR_DEV_INDEX]); > > diff --git a/include/uapi/rdma/rdma_netlink.h > > b/include/uapi/rdma/rdma_netlink.h > > index b27c02185dcc..24ff4ffa30dd 100644 > > +++ b/include/uapi/rdma/rdma_netlink.h > > @@ -285,6 +285,7 @@ enum rdma_nldev_command { > > }; > > > > enum { > > + RDMA_NLDEV_ATTR_EMPTY_STRING = 1, > > RDMA_NLDEV_ATTR_ENTRY_STRLEN = 16, > > The empty thing is just an internal implementation detail, should not > leak into uapi So was ENTRY_STRLEN. Once I audited and set all non-input strings to EMPTY_STRING, ENTRY_STRLEN isn't even used any more. It happens to be the same as IFNAMSIZ and so could be used in its place, but doesn't have to be.
On Wed, Jun 19, 2019 at 04:02:30PM -0400, Doug Ledford wrote: > On Wed, 2019-06-19 at 16:24 -0300, Jason Gunthorpe wrote: > > > > > + nla_strlcpy(client_name, tb[RDMA_NLDEV_ATTR_CHARDEV_TYPE], > > > + sizeof(client_name)); > > > > This seems really frail, it should at least have a > > > > BUILD_BUG_ON(sizeof(client_name) == > > nldev_policy[RDMA_NLDEV_ATTR_CHARDEV_TYPE].len)); > > > > But I don't think that can compile. > > > > Are we sure we can't have a 0 length and just skip checking in policy? > > It seems like more danger than it is worth. > > nla_strlcpy takes a size parameter as arg3 and guarantees to put no > more than arg3 bytes - 1 from the source into the dest and > guarantees it's null terminated. The only difference between > nla_strlcpy and normal strncpy is that nla_strlcpy zeros out any > excess pad bytes that aren't used in the dest by the copy. If > someone tries to pass in an oversized string, it doesn't matter. If > someone modifes the function to change the size of client_name, our > nla_strlcpy() is safe because our dest and our len parameters are > always in sync. I don't see the fragility. Silently truncating the user input string and not returning an error is a kernel bug. I dislike strlcpy and friends for the same reason everyone else does - they prevent security failures but create bugs with undetected truncated strings. > > > if (tb[RDMA_NLDEV_ATTR_DEV_INDEX]) { > > > index = nla_get_u32(tb[RDMA_NLDEV_ATTR_DEV_INDEX]); > > > diff --git a/include/uapi/rdma/rdma_netlink.h > > > b/include/uapi/rdma/rdma_netlink.h > > > index b27c02185dcc..24ff4ffa30dd 100644 > > > +++ b/include/uapi/rdma/rdma_netlink.h > > > @@ -285,6 +285,7 @@ enum rdma_nldev_command { > > > }; > > > > > > enum { > > > + RDMA_NLDEV_ATTR_EMPTY_STRING = 1, > > > RDMA_NLDEV_ATTR_ENTRY_STRLEN = 16, > > > > The empty thing is just an internal implementation detail, should not > > leak into uapi > > So was ENTRY_STRLEN. Once I audited and set all non-input strings to > EMPTY_STRING, ENTRY_STRLEN isn't even used any more. It happens to be > the same as IFNAMSIZ and so could be used in its place, but doesn't have > to be. Should move both then Jason
On Wed, 2019-06-19 at 17:11 -0300, Jason Gunthorpe wrote: > On Wed, Jun 19, 2019 at 04:02:30PM -0400, Doug Ledford wrote: > > On Wed, 2019-06-19 at 16:24 -0300, Jason Gunthorpe wrote: > > > > + nla_strlcpy(client_name, > > > > tb[RDMA_NLDEV_ATTR_CHARDEV_TYPE], > > > > + sizeof(client_name)); > > > > > > This seems really frail, it should at least have a > > > > > > BUILD_BUG_ON(sizeof(client_name) == > > > nldev_policy[RDMA_NLDEV_ATTR_CHARDEV_TYPE].len)); > > > > > > But I don't think that can compile. > > > > > > Are we sure we can't have a 0 length and just skip checking in > > > policy? > > > It seems like more danger than it is worth. > > > > nla_strlcpy takes a size parameter as arg3 and guarantees to put no > > more than arg3 bytes - 1 from the source into the dest and > > guarantees it's null terminated. The only difference between > > nla_strlcpy and normal strncpy is that nla_strlcpy zeros out any > > excess pad bytes that aren't used in the dest by the copy. If > > someone tries to pass in an oversized string, it doesn't matter. If > > someone modifes the function to change the size of client_name, our > > nla_strlcpy() is safe because our dest and our len parameters are > > always in sync. I don't see the fragility. > > Silently truncating the user input string and not returning an error > is a kernel bug. That's a matter of expectations. Looking through all instances of nla_strlcpy() in the kernel tree, only about 4 of the 25 or so uses check the return code at all, and only 1 of those returns an error code to the application. This mirrors the behavior people see when they try to do things like rename a netdevice and pass in too long of a name. As long as the truncated name isn't taken, it succeeds at the truncated name. > I dislike strlcpy and friends for the same reason everyone else does - > they prevent security failures but create bugs with undetected > truncated strings. I think the net stack has a fairly well established behavior in regards to truncating overly long strings. Sending an error is just as likely to break people's assumptions that even an overly long name will succeed as long as the truncated version of the name isn't taken. Regardless though, I think it's fair to say the current code as this patch makes it is not fragile. It is the opposite. It just happens to follow netdev behavior, and that standard offends your better sensibilities ;-). The question is whether it is better to buck the standard and risk confusing people that are actually used to silent truncation, or fix the perceived kernel bug? I don't actually feel strongly about it. I'm used to the truncation behavior. If you would strongly prefer an error on overflow, I'll change the patch up. > > > > if (tb[RDMA_NLDEV_ATTR_DEV_INDEX]) { > > > > index = > > > > nla_get_u32(tb[RDMA_NLDEV_ATTR_DEV_INDEX]); > > > > diff --git a/include/uapi/rdma/rdma_netlink.h > > > > b/include/uapi/rdma/rdma_netlink.h > > > > index b27c02185dcc..24ff4ffa30dd 100644 > > > > +++ b/include/uapi/rdma/rdma_netlink.h > > > > @@ -285,6 +285,7 @@ enum rdma_nldev_command { > > > > }; > > > > > > > > enum { > > > > + RDMA_NLDEV_ATTR_EMPTY_STRING = 1, > > > > RDMA_NLDEV_ATTR_ENTRY_STRLEN = 16, > > > > > > The empty thing is just an internal implementation detail, should > > > not > > > leak into uapi > > > > So was ENTRY_STRLEN. Once I audited and set all non-input strings > > to > > EMPTY_STRING, ENTRY_STRLEN isn't even used any more. It happens to > > be > > the same as IFNAMSIZ and so could be used in its place, but doesn't > > have > > to be. > > Should move both then Done.
On Wed, Jun 19, 2019 at 07:59:22PM -0400, Doug Ledford wrote: > On Wed, 2019-06-19 at 17:11 -0300, Jason Gunthorpe wrote: > > On Wed, Jun 19, 2019 at 04:02:30PM -0400, Doug Ledford wrote: > > > On Wed, 2019-06-19 at 16:24 -0300, Jason Gunthorpe wrote: > > > > > + nla_strlcpy(client_name, > > > > > tb[RDMA_NLDEV_ATTR_CHARDEV_TYPE], > > > > > + sizeof(client_name)); > > > > > > > > This seems really frail, it should at least have a > > > > > > > > BUILD_BUG_ON(sizeof(client_name) == > > > > nldev_policy[RDMA_NLDEV_ATTR_CHARDEV_TYPE].len)); > > > > > > > > But I don't think that can compile. > > > > > > > > Are we sure we can't have a 0 length and just skip checking in > > > > policy? > > > > It seems like more danger than it is worth. > > > > > > nla_strlcpy takes a size parameter as arg3 and guarantees to put no > > > more than arg3 bytes - 1 from the source into the dest and > > > guarantees it's null terminated. The only difference between > > > nla_strlcpy and normal strncpy is that nla_strlcpy zeros out any > > > excess pad bytes that aren't used in the dest by the copy. If > > > someone tries to pass in an oversized string, it doesn't matter. If > > > someone modifes the function to change the size of client_name, our > > > nla_strlcpy() is safe because our dest and our len parameters are > > > always in sync. I don't see the fragility. > > > > Silently truncating the user input string and not returning an error > > is a kernel bug. > > That's a matter of expectations. Looking through all instances of > nla_strlcpy() in the kernel tree, only about 4 of the 25 or so uses > check the return code at all Sadly the kernel is full of bugs :( Silent string truncation is a well known bug class, I guess Dan Carpenter just hasn't quite got to sending reports on these cases yet.. > and only 1 of those returns an error code to the application. This > mirrors the behavior people see when they try to do things like > rename a netdevice and pass in too long of a name. As long as the > truncated name isn't taken, it succeeds at the truncated name. Returning ENOENT, or worse, the entirely wrong result instead of EINVAL, for a too long string is a straight up bug. The probability of any user hitting this is very low, but it is not some well thought out behavior... > > I dislike strlcpy and friends for the same reason everyone else does - > > they prevent security failures but create bugs with undetected > > truncated strings. > > I think the net stack has a fairly well established behavior in regards > to truncating overly long strings. I think it is just bugs. :( > truncation, or fix the perceived kernel bug? I don't actually feel > strongly about it. I'm used to the truncation behavior. If you would > strongly prefer an error on overflow, I'll change the patch up. The client_name is a stack variable of a completely arbitrary size that is larger than all existing client_name statics in today's kernel. Tomorrow it may get bigger, or smaller. Its length absolutely does not form part of the uAPI contract, and truncation of the user provided string should always return EINVAL never any other result. Jason
On Wed, 2019-06-19 at 21:17 -0300, Jason Gunthorpe wrote: > On Wed, Jun 19, 2019 at 07:59:22PM -0400, Doug Ledford wrote: > > On Wed, 2019-06-19 at 17:11 -0300, Jason Gunthorpe wrote: > > > On Wed, Jun 19, 2019 at 04:02:30PM -0400, Doug Ledford wrote: > > > > On Wed, 2019-06-19 at 16:24 -0300, Jason Gunthorpe wrote: > > > > > > + nla_strlcpy(client_name, > > > > > > tb[RDMA_NLDEV_ATTR_CHARDEV_TYPE], > > > > > > + sizeof(client_name)); > > > > > > > > > > This seems really frail, it should at least have a > > > > > > > > > > BUILD_BUG_ON(sizeof(client_name) == > > > > > nldev_policy[RDMA_NLDEV_ATTR_CHARDEV_TYPE].len)); > > > > > > > > > > But I don't think that can compile. > > > > > > > > > > Are we sure we can't have a 0 length and just skip checking in > > > > > policy? > > > > > It seems like more danger than it is worth. > > > > > > > > nla_strlcpy takes a size parameter as arg3 and guarantees to put > > > > no > > > > more than arg3 bytes - 1 from the source into the dest and > > > > guarantees it's null terminated. The only difference between > > > > nla_strlcpy and normal strncpy is that nla_strlcpy zeros out any > > > > excess pad bytes that aren't used in the dest by the copy. If > > > > someone tries to pass in an oversized string, it doesn't > > > > matter. If > > > > someone modifes the function to change the size of client_name, > > > > our > > > > nla_strlcpy() is safe because our dest and our len parameters > > > > are > > > > always in sync. I don't see the fragility. > > > > > > Silently truncating the user input string and not returning an > > > error > > > is a kernel bug. > > > > That's a matter of expectations. Looking through all instances of > > nla_strlcpy() in the kernel tree, only about 4 of the 25 or so uses > > check the return code at all > > Sadly the kernel is full of bugs :( > > Silent string truncation is a well known bug class, I guess Dan > Carpenter just hasn't quite got to sending reports on these cases > yet.. > > > and only 1 of those returns an error code to the application. This > > mirrors the behavior people see when they try to do things like > > rename a netdevice and pass in too long of a name. As long as the > > truncated name isn't taken, it succeeds at the truncated name. > > Returning ENOENT, or worse, the entirely wrong result instead of > EINVAL, for a too long string is a straight up bug. > > The probability of any user hitting this is very low, but it is not > some well thought out behavior... > > > > I dislike strlcpy and friends for the same reason everyone else > > > does - > > > they prevent security failures but create bugs with undetected > > > truncated strings. > > > > I think the net stack has a fairly well established behavior in > > regards > > to truncating overly long strings. > > I think it is just bugs. :( > > > truncation, or fix the perceived kernel bug? I don't actually feel > > strongly about it. I'm used to the truncation behavior. If you > > would > > strongly prefer an error on overflow, I'll change the patch up. > > The client_name is a stack variable of a completely arbitrary size > that > is larger than all existing client_name statics in today's > kernel. Tomorrow it may get bigger, or smaller. Its length absolutely > does not form part of the uAPI contract, and truncation of the user > provided string should always return EINVAL never any other result. Ok. I'll push out everything but this patch, and rework this patch tomorrow.
diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c index 6006d23d0410..291d13642fcf 100644 --- a/drivers/infiniband/core/nldev.c +++ b/drivers/infiniband/core/nldev.c @@ -49,29 +49,29 @@ static const struct nla_policy nldev_policy[RDMA_NLDEV_ATTR_MAX] = { [RDMA_NLDEV_ATTR_CHARDEV] = { .type = NLA_U64 }, [RDMA_NLDEV_ATTR_CHARDEV_ABI] = { .type = NLA_U64 }, [RDMA_NLDEV_ATTR_CHARDEV_NAME] = { .type = NLA_NUL_STRING, - .len = RDMA_NLDEV_ATTR_ENTRY_STRLEN }, + .len = RDMA_NLDEV_ATTR_EMPTY_STRING }, [RDMA_NLDEV_ATTR_CHARDEV_TYPE] = { .type = NLA_NUL_STRING, - .len = 128 }, + .len = IB_DEVICE_NAME_MAX }, [RDMA_NLDEV_ATTR_DEV_INDEX] = { .type = NLA_U32 }, [RDMA_NLDEV_ATTR_DEV_NAME] = { .type = NLA_NUL_STRING, - .len = IB_DEVICE_NAME_MAX - 1}, + .len = IB_DEVICE_NAME_MAX }, [RDMA_NLDEV_ATTR_DEV_NODE_TYPE] = { .type = NLA_U8 }, [RDMA_NLDEV_ATTR_DEV_PROTOCOL] = { .type = NLA_NUL_STRING, - .len = RDMA_NLDEV_ATTR_ENTRY_STRLEN }, + .len = RDMA_NLDEV_ATTR_EMPTY_STRING }, [RDMA_NLDEV_ATTR_DRIVER] = { .type = NLA_NESTED }, [RDMA_NLDEV_ATTR_DRIVER_ENTRY] = { .type = NLA_NESTED }, [RDMA_NLDEV_ATTR_DRIVER_PRINT_TYPE] = { .type = NLA_U8 }, [RDMA_NLDEV_ATTR_DRIVER_STRING] = { .type = NLA_NUL_STRING, - .len = RDMA_NLDEV_ATTR_ENTRY_STRLEN }, + .len = RDMA_NLDEV_ATTR_EMPTY_STRING }, [RDMA_NLDEV_ATTR_DRIVER_S32] = { .type = NLA_S32 }, [RDMA_NLDEV_ATTR_DRIVER_S64] = { .type = NLA_S64 }, [RDMA_NLDEV_ATTR_DRIVER_U32] = { .type = NLA_U32 }, [RDMA_NLDEV_ATTR_DRIVER_U64] = { .type = NLA_U64 }, [RDMA_NLDEV_ATTR_FW_VERSION] = { .type = NLA_NUL_STRING, - .len = IB_FW_VERSION_NAME_MAX - 1}, + .len = RDMA_NLDEV_ATTR_EMPTY_STRING }, [RDMA_NLDEV_ATTR_LID] = { .type = NLA_U32 }, [RDMA_NLDEV_ATTR_LINK_TYPE] = { .type = NLA_NUL_STRING, - .len = RDMA_NLDEV_ATTR_ENTRY_STRLEN }, + .len = IFNAMSIZ }, [RDMA_NLDEV_ATTR_LMC] = { .type = NLA_U8 }, [RDMA_NLDEV_ATTR_NDEV_INDEX] = { .type = NLA_U32 }, [RDMA_NLDEV_ATTR_NDEV_NAME] = { .type = NLA_NUL_STRING, @@ -92,7 +92,7 @@ static const struct nla_policy nldev_policy[RDMA_NLDEV_ATTR_MAX] = { .len = sizeof(struct __kernel_sockaddr_storage) }, [RDMA_NLDEV_ATTR_RES_IOVA] = { .type = NLA_U64 }, [RDMA_NLDEV_ATTR_RES_KERN_NAME] = { .type = NLA_NUL_STRING, - .len = TASK_COMM_LEN }, + .len = RDMA_NLDEV_ATTR_EMPTY_STRING }, [RDMA_NLDEV_ATTR_RES_LKEY] = { .type = NLA_U32 }, [RDMA_NLDEV_ATTR_RES_LOCAL_DMA_LKEY] = { .type = NLA_U32 }, [RDMA_NLDEV_ATTR_RES_LQPN] = { .type = NLA_U32 }, @@ -120,7 +120,7 @@ static const struct nla_policy nldev_policy[RDMA_NLDEV_ATTR_MAX] = { [RDMA_NLDEV_ATTR_RES_SUMMARY_ENTRY] = { .type = NLA_NESTED }, [RDMA_NLDEV_ATTR_RES_SUMMARY_ENTRY_CURR]= { .type = NLA_U64 }, [RDMA_NLDEV_ATTR_RES_SUMMARY_ENTRY_NAME]= { .type = NLA_NUL_STRING, - .len = 16 }, + .len = RDMA_NLDEV_ATTR_EMPTY_STRING }, [RDMA_NLDEV_ATTR_RES_TYPE] = { .type = NLA_U8 }, [RDMA_NLDEV_ATTR_RES_UNSAFE_GLOBAL_RKEY]= { .type = NLA_U32 }, [RDMA_NLDEV_ATTR_RES_USECNT] = { .type = NLA_U64 }, @@ -1372,10 +1372,8 @@ static int nldev_get_chardev(struct sk_buff *skb, struct nlmsghdr *nlh, extack); if (err || !tb[RDMA_NLDEV_ATTR_CHARDEV_TYPE]) return -EINVAL; - - if (nla_strlcpy(client_name, tb[RDMA_NLDEV_ATTR_CHARDEV_TYPE], - sizeof(client_name)) >= sizeof(client_name)) - return -EINVAL; + nla_strlcpy(client_name, tb[RDMA_NLDEV_ATTR_CHARDEV_TYPE], + sizeof(client_name)); if (tb[RDMA_NLDEV_ATTR_DEV_INDEX]) { index = nla_get_u32(tb[RDMA_NLDEV_ATTR_DEV_INDEX]); diff --git a/include/uapi/rdma/rdma_netlink.h b/include/uapi/rdma/rdma_netlink.h index b27c02185dcc..24ff4ffa30dd 100644 --- a/include/uapi/rdma/rdma_netlink.h +++ b/include/uapi/rdma/rdma_netlink.h @@ -285,6 +285,7 @@ enum rdma_nldev_command { }; enum { + RDMA_NLDEV_ATTR_EMPTY_STRING = 1, RDMA_NLDEV_ATTR_ENTRY_STRLEN = 16, };
For all string attributes for which we don't currently accept the element as input, we only use it as output, set the string length to RDMA_NLDEV_ATTR_EMPTY_STRING which is defined as 1. That way we will only accept a null string for that element. This will prevent someone from writing a new input routine that uses the element without also updating the policy to have a valid value. Also while there, make sure the existing entries that are valid have the correct policy, if not, correct the policy. Remove unnecessary checks for nla_strlcpy() overflow once the policy has been set correctly. Signed-off-by: Doug Ledford <dledford@redhat.com> --- drivers/infiniband/core/nldev.c | 24 +++++++++++------------- include/uapi/rdma/rdma_netlink.h | 1 + 2 files changed, 12 insertions(+), 13 deletions(-) v0->v1: Remove all whitespace change noise from this patch, this patch is now all functional changes