| Message ID | 20191011133525.GB22905@mwanda (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | USB: legousbtower: fix a signedness bug in tower_probe() | expand |
Am 11.10.2019 15:35, schrieb Dan Carpenter: > The problem is that sizeof() is unsigned long so negative error codes > are type promoted to high positive values and the condition becomes > false. > > Fixes: 1d427be4a39d ("USB: legousbtower: fix slab info leak at probe") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > drivers/usb/misc/legousbtower.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c > index 9d4c52a7ebe0..835908fe1e65 100644 > --- a/drivers/usb/misc/legousbtower.c > +++ b/drivers/usb/misc/legousbtower.c > @@ -881,7 +881,7 @@ static int tower_probe (struct usb_interface *interface, const struct usb_device > get_version_reply, > sizeof(*get_version_reply), > 1000); > - if (result < sizeof(*get_version_reply)) { > + if (result < 0 || result < sizeof(*get_version_reply)) { > if (result >= 0) > result = -EIO; > dev_err(idev, "get version request failed: %d\n", result); i am not an USB expert but it seems that this is a complicated way to check for result != sizeof(*get_version_reply). re, wh
On Fri, Oct 11, 2019 at 03:51:26PM +0200, walter harms wrote: > > > Am 11.10.2019 15:35, schrieb Dan Carpenter: > > The problem is that sizeof() is unsigned long so negative error codes > > are type promoted to high positive values and the condition becomes > > false. > > > > Fixes: 1d427be4a39d ("USB: legousbtower: fix slab info leak at probe") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > drivers/usb/misc/legousbtower.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c > > index 9d4c52a7ebe0..835908fe1e65 100644 > > --- a/drivers/usb/misc/legousbtower.c > > +++ b/drivers/usb/misc/legousbtower.c > > @@ -881,7 +881,7 @@ static int tower_probe (struct usb_interface *interface, const struct usb_device > > get_version_reply, > > sizeof(*get_version_reply), > > 1000); > > - if (result < sizeof(*get_version_reply)) { > > + if (result < 0 || result < sizeof(*get_version_reply)) { > > if (result >= 0) > > result = -EIO; > > dev_err(idev, "get version request failed: %d\n", result); > > i am not an USB expert but it seems that this is a complicated way > to check for result != sizeof(*get_version_reply). Yeah. You're right. That would look nicer. I will resend. regards, dan carpenter
On Fri, Oct 11, 2019 at 04:35:25PM +0300, Dan Carpenter wrote: > The problem is that sizeof() is unsigned long so negative error codes > are type promoted to high positive values and the condition becomes > false. > > Fixes: 1d427be4a39d ("USB: legousbtower: fix slab info leak at probe") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > drivers/usb/misc/legousbtower.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c > index 9d4c52a7ebe0..835908fe1e65 100644 > --- a/drivers/usb/misc/legousbtower.c > +++ b/drivers/usb/misc/legousbtower.c > @@ -881,7 +881,7 @@ static int tower_probe (struct usb_interface *interface, const struct usb_device > get_version_reply, > sizeof(*get_version_reply), > 1000); > - if (result < sizeof(*get_version_reply)) { > + if (result < 0 || result < sizeof(*get_version_reply)) { > if (result >= 0) > result = -EIO; > dev_err(idev, "get version request failed: %d\n", result); Bah, I should have noticed. Thanks for fixing this! Acked-by: Johan Hovold <johan@kernel.org> Johan
On Fri, Oct 11, 2019 at 04:58:56PM +0300, Dan Carpenter wrote: > On Fri, Oct 11, 2019 at 03:51:26PM +0200, walter harms wrote: > > > > > > Am 11.10.2019 15:35, schrieb Dan Carpenter: > > > The problem is that sizeof() is unsigned long so negative error codes > > > are type promoted to high positive values and the condition becomes > > > false. > > > > > > Fixes: 1d427be4a39d ("USB: legousbtower: fix slab info leak at probe") > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > --- > > > drivers/usb/misc/legousbtower.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c > > > index 9d4c52a7ebe0..835908fe1e65 100644 > > > --- a/drivers/usb/misc/legousbtower.c > > > +++ b/drivers/usb/misc/legousbtower.c > > > @@ -881,7 +881,7 @@ static int tower_probe (struct usb_interface *interface, const struct usb_device > > > get_version_reply, > > > sizeof(*get_version_reply), > > > 1000); > > > - if (result < sizeof(*get_version_reply)) { > > > + if (result < 0 || result < sizeof(*get_version_reply)) { > > > if (result >= 0) > > > result = -EIO; > > > dev_err(idev, "get version request failed: %d\n", result); > > > > i am not an USB expert but it seems that this is a complicated way > > to check for result != sizeof(*get_version_reply). > > Yeah. You're right. That would look nicer. I will resend. Your version, or adding an explicit cast to int, may be preferred as they document that there's something to watch out for here. Either way you have my ack. Johan
diff --git a/drivers/usb/misc/legousbtower.c b/drivers/usb/misc/legousbtower.c index 9d4c52a7ebe0..835908fe1e65 100644 --- a/drivers/usb/misc/legousbtower.c +++ b/drivers/usb/misc/legousbtower.c @@ -881,7 +881,7 @@ static int tower_probe (struct usb_interface *interface, const struct usb_device get_version_reply, sizeof(*get_version_reply), 1000); - if (result < sizeof(*get_version_reply)) { + if (result < 0 || result < sizeof(*get_version_reply)) { if (result >= 0) result = -EIO; dev_err(idev, "get version request failed: %d\n", result);
The problem is that sizeof() is unsigned long so negative error codes are type promoted to high positive values and the condition becomes false. Fixes: 1d427be4a39d ("USB: legousbtower: fix slab info leak at probe") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- drivers/usb/misc/legousbtower.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)