> -----Original Message----- > From: Jason Gunthorpe <jgg@xxxxxxxx> > Sent: Wednesday, April 24, 2019 9:24 AM > To: Dan Carpenter <dan.carpenter@xxxxxxxxxx> > Cc: Parav Pandit <parav@xxxxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; Leon > Romanovsky <leon@xxxxxxxxxx>; Eli Cohen <eli@xxxxxxxxxxxx>; Doug > Ledford <dledford@xxxxxxxxxx>; linux-rdma@xxxxxxxxxxxxxxx; kernel- > janitors@xxxxxxxxxxxxxxx > Subject: Re: [PATCH] IB/mlx5: add checking for "vf" from do_setvfinfo() > > On Wed, Apr 24, 2019 at 05:08:20PM +0300, Dan Carpenter wrote: > > I think I'm just going to ask netdev for an opinion on this. It could > > be that we're just reading the code wrong... > > I don't think you are reading it wrong. > > Allowing the compiler to implicitly cast a user controlled u32 to an int is > simply wrong in all cases, IMHO. > > If the value was intended to be signed from the user it should have been a > s32. Allowing an unsigned value to become interpreted as negative so often > leads to security bugs. > > IMHO it would be a good thing for smatch to warn on the general case of > implicit casting of user controlled data to a smaller range type. Particularly it > can do a bounds analysis to show the control flow hasn't somehow > restricted the bounds to be compatible. > > I've seen more that a few real world security bugs that are caused by wrong > use of 'int' like this :( > > Jason Hence we should fix the type to be u32 in ndo ops to match netlink type core and in driver, instead of < 0 check.
