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.
--
Doug Ledford <dledford@xxxxxxxxxx>
GPG KeyID: B826A3330E572FDD
Fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
Attachment:
signature.asc
Description: This is a digitally signed message part
