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
