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.
--
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
