Comparing an int to a size, which is unsigned, causes the int to become
unsigned, giving the wrong result.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@safe disable not_int2@
int x;
position p;
binary operator op = {<,<=};
expression e;
@@
(
x < 0 || (x@p op e)
|
x <= 0 || (x@p op e)
|
x > 0 && (x@p op e)
|
x >= 0 && (x@p op e)
)
@@
int x;
type t;
expression e,e1;
identifier f != {strlen,resource_size};
position p != safe.p;
binary operator op = {<,<=};
@@
*x = f(...);
... when != x = e1
when != if (x < 0 || ...) { ... return ...; }
(
*x@p op sizeof(e)
|
*x@p op sizeof(t)
)
// </smpl>
---
drivers/input/mouse/elan_i2c_smbus.c | 2 +-
drivers/media/usb/gspca/kinect.c | 2 +-
drivers/usb/wusbcore/security.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
Comparing an int to a size, which is unsigned, causes the int to become
unsigned, giving the wrong result. i2c_smbus_read_block_data can return the
result of i2c_smbus_xfer, whih can return a negative error code.
A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)
// <smpl>
@@
int x;
expression e,e1;
identifier f;
@@
*x = f(...);
... when != x = e1
when != if (x < 0 || ...) { ... return ...; }
*x < sizeof(e)
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
drivers/input/mouse/elan_i2c_smbus.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/mouse/elan_i2c_smbus.c b/drivers/input/mouse/elan_i2c_smbus.c
index c060d27..88e315d 100644
--- a/drivers/input/mouse/elan_i2c_smbus.c
+++ b/drivers/input/mouse/elan_i2c_smbus.c
@@ -387,7 +387,7 @@ static int elan_smbus_prepare_fw_update(struct i2c_client *client)
len = i2c_smbus_read_block_data(client,
ETP_SMBUS_IAP_PASSWORD_READ,
val);
- if (len < sizeof(u16)) {
+ if (len < (int)sizeof(u16)) {
error = len < 0 ? len : -EIO;
dev_err(dev, "failed to read iap password: %d\n",
error);
Comparing an int to a size, which is unsigned, causes the int to become
unsigned, giving the wrong result. usb_get_descriptor can return a
negative error code.
A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)
// <smpl>
@@
int x;
expression e,e1;
identifier f;
@@
*x = f(...);
... when != x = e1
when != if (x < 0 || ...) { ... return ...; }
*x < sizeof(e)
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
drivers/usb/wusbcore/security.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/wusbcore/security.c b/drivers/usb/wusbcore/security.c
index 33d2f5d..14ac8c9 100644
--- a/drivers/usb/wusbcore/security.c
+++ b/drivers/usb/wusbcore/security.c
@@ -217,7 +217,7 @@ int wusb_dev_sec_add(struct wusbhc *wusbhc,
result = usb_get_descriptor(usb_dev, USB_DT_SECURITY,
0, secd, sizeof(*secd));
- if (result < sizeof(*secd)) {
+ if (result < (int)sizeof(*secd)) {
dev_err(dev, "Can't read security descriptor or "
"not enough data: %d\n", result);
goto out;
Comparing an int to a size, which is unsigned, causes the int to become
unsigned, giving the wrong result. kinect_read returns the result of
usb_control_msg, which can return a negtive error code.
A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)
// <smpl>
@@
int x;
expression e,e1;
identifier f;
@@
*x = f(...);
... when != x = e1
when != if (x < 0 || ...) { ... return ...; }
*x < sizeof(e)
// </smpl>
Signed-off-by: Julia Lawall <[email protected]>
---
drivers/media/usb/gspca/kinect.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/usb/gspca/kinect.c b/drivers/media/usb/gspca/kinect.c
index 0cfdf8a..f993f62 100644
--- a/drivers/media/usb/gspca/kinect.c
+++ b/drivers/media/usb/gspca/kinect.c
@@ -163,7 +163,7 @@ static int send_cmd(struct gspca_dev *gspca_dev, uint16_t cmd, void *cmdbuf,
actual_len = kinect_read(udev, ibuf, 0x200);
} while (actual_len == 0);
gspca_dbg(gspca_dev, D_USBO, "Control reply: %d\n", actual_len);
- if (actual_len < sizeof(*rhdr)) {
+ if (actual_len < (int)sizeof(*rhdr)) {
pr_err("send_cmd: Input control transfer failed (%d)\n",
actual_len);
return actual_len < 0 ? actual_len : -EREMOTEIO;
On Sun, 2018-07-01 at 19:32 +0200, Julia Lawall wrote:
> Comparing an int to a size, which is unsigned, causes the int to become
> unsigned, giving the wrong result.
>
> The semantic match that finds this problem is as follows:
> (http://coccinelle.lip6.fr/)
Great, thanks.
But what about the ones in net/smc like:
> net/smc/smc_clc.c:
>
> len = kernel_sendmsg(smc->clcsock, &msg, &vec, 1,
> sizeof(struct smc_clc_msg_decline));
> if (len < sizeof(struct smc_clc_msg_decline))
Are those detected by the semantic match and ignored?
On Sun, 1 Jul 2018, Joe Perches wrote:
> On Sun, 2018-07-01 at 19:32 +0200, Julia Lawall wrote:
> > Comparing an int to a size, which is unsigned, causes the int to become
> > unsigned, giving the wrong result.
> >
> > The semantic match that finds this problem is as follows:
> > (http://coccinelle.lip6.fr/)
>
> Great, thanks.
>
> But what about the ones in net/smc like:
>
> > net/smc/smc_clc.c:
> >
> > len = kernel_sendmsg(smc->clcsock, &msg, &vec, 1,
> > sizeof(struct smc_clc_msg_decline));
> > if (len < sizeof(struct smc_clc_msg_decline))
>
> Are those detected by the semantic match and ignored?
I wasn't sure how to justify that kernel_sendmsg returns a negative value.
If it is the case, I can send the patch. I only found this in one file,
but there were multiple occurrences.
julia
On Sun, Jul 01, 2018 at 08:51:55PM +0200, Julia Lawall wrote:
>
>
> On Sun, 1 Jul 2018, Joe Perches wrote:
>
> > On Sun, 2018-07-01 at 19:32 +0200, Julia Lawall wrote:
> > > Comparing an int to a size, which is unsigned, causes the int to become
> > > unsigned, giving the wrong result.
> > >
> > > The semantic match that finds this problem is as follows:
> > > (http://coccinelle.lip6.fr/)
> >
> > Great, thanks.
> >
> > But what about the ones in net/smc like:
> >
> > > net/smc/smc_clc.c:
> > >
> > > len = kernel_sendmsg(smc->clcsock, &msg, &vec, 1,
> > > sizeof(struct smc_clc_msg_decline));
> > > if (len < sizeof(struct smc_clc_msg_decline))
> >
> > Are those detected by the semantic match and ignored?
>
> I wasn't sure how to justify that kernel_sendmsg returns a negative value.
> If it is the case, I can send the patch. I only found this in one file,
> but there were multiple occurrences.
>
In theory, Smatch is supposed to know return values but kernel_sendmsg()
is too complicated for Smatch. It's a tricky thing... That particular
check is correct and deliberate, but there is another check which is
wrong.
net/smc/smc_clc.c
369 len = kernel_sendmsg(smc->clcsock, &msg, &vec, 1,
370 sizeof(struct smc_clc_msg_decline));
371 if (len < sizeof(struct smc_clc_msg_decline))
372 smc->sk.sk_err = EPROTO;
373 if (len < 0)
374 smc->sk.sk_err = -len;
If it's invalid we set an error code, if it's already an error we
preserve the error code.
375 return sock_error(&smc->sk);
[ snip ]
442 /* due to the few bytes needed for clc-handshake this cannot block */
443 len = kernel_sendmsg(smc->clcsock, &msg, vec, i, plen);
444 if (len < sizeof(pclc)) {
445 if (len >= 0) {
^^^^^^^^
This is always true.
446 reason_code = -ENETUNREACH;
447 smc->sk.sk_err = -reason_code;
448 } else {
449 smc->sk.sk_err = smc->clcsock->sk->sk_err;
450 reason_code = -smc->sk.sk_err;
451 }
452 }
The other two checks are not type promoted so they also work as
intended.
This is an interesting sort of bug I've written a Smatch script inspired
by your work here. One for the type promotion and one for the
impossible condition. I'll let you know how it goes.
regards,
dan carpenter
On Sun, Jul 01, 2018 at 07:32:03PM +0200, Julia Lawall wrote:
> Comparing an int to a size, which is unsigned, causes the int to become
> unsigned, giving the wrong result. i2c_smbus_read_block_data can return the
> result of i2c_smbus_xfer, whih can return a negative error code.
>
> A simplified version of the semantic match that finds this problem is as
> follows: (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @@
> int x;
> expression e,e1;
> identifier f;
> @@
>
> *x = f(...);
> ... when != x = e1
> when != if (x < 0 || ...) { ... return ...; }
> *x < sizeof(e)
> // </smpl>
>
> Signed-off-by: Julia Lawall <[email protected]>
>
Applied, thank you.
> ---
> drivers/input/mouse/elan_i2c_smbus.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/input/mouse/elan_i2c_smbus.c b/drivers/input/mouse/elan_i2c_smbus.c
> index c060d27..88e315d 100644
> --- a/drivers/input/mouse/elan_i2c_smbus.c
> +++ b/drivers/input/mouse/elan_i2c_smbus.c
> @@ -387,7 +387,7 @@ static int elan_smbus_prepare_fw_update(struct i2c_client *client)
> len = i2c_smbus_read_block_data(client,
> ETP_SMBUS_IAP_PASSWORD_READ,
> val);
> - if (len < sizeof(u16)) {
> + if (len < (int)sizeof(u16)) {
> error = len < 0 ? len : -EIO;
> dev_err(dev, "failed to read iap password: %d\n",
> error);
>
--
Dmitry