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