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
