* [patch] netiucv: silence an underflow warning @ 2016-07-14 11:30 ` Dan Carpenter 0 siblings, 0 replies; 6+ messages in thread From: Dan Carpenter @ 2016-07-14 11:30 UTC (permalink / raw) To: Ursula Braun Cc: Martin Schwidefsky, Heiko Carstens, linux-s390, linux-kernel, kernel-janitors I haven't looked at the implications but we accidentally allow bs1 to be negative. It makes my static checker complain. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> diff --git a/drivers/s390/net/netiucv.c b/drivers/s390/net/netiucv.c index b0e8ffd..85a5744 100644 --- a/drivers/s390/net/netiucv.c +++ b/drivers/s390/net/netiucv.c @@ -1578,7 +1578,7 @@ static ssize_t buffer_write (struct device *dev, struct device_attribute *attr, *e); return -EINVAL; } - if (bs1 > NETIUCV_BUFSIZE_MAX) { + if (bs1 < 0 || bs1 > NETIUCV_BUFSIZE_MAX) { IUCV_DBF_TEXT_(setup, 2, "buffer_write: buffer size %d too large\n", bs1); ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [patch] netiucv: silence an underflow warning @ 2016-07-14 11:30 ` Dan Carpenter 0 siblings, 0 replies; 6+ messages in thread From: Dan Carpenter @ 2016-07-14 11:30 UTC (permalink / raw) To: Ursula Braun Cc: Martin Schwidefsky, Heiko Carstens, linux-s390, linux-kernel, kernel-janitors I haven't looked at the implications but we accidentally allow bs1 to be negative. It makes my static checker complain. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> diff --git a/drivers/s390/net/netiucv.c b/drivers/s390/net/netiucv.c index b0e8ffd..85a5744 100644 --- a/drivers/s390/net/netiucv.c +++ b/drivers/s390/net/netiucv.c @@ -1578,7 +1578,7 @@ static ssize_t buffer_write (struct device *dev, struct device_attribute *attr, *e); return -EINVAL; } - if (bs1 > NETIUCV_BUFSIZE_MAX) { + if (bs1 < 0 || bs1 > NETIUCV_BUFSIZE_MAX) { IUCV_DBF_TEXT_(setup, 2, "buffer_write: buffer size %d too large\n", bs1); ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [patch] netiucv: silence an underflow warning 2016-07-14 11:30 ` Dan Carpenter @ 2016-07-15 7:11 ` Ursula Braun -1 siblings, 0 replies; 6+ messages in thread From: Ursula Braun @ 2016-07-15 7:11 UTC (permalink / raw) To: Dan Carpenter Cc: Martin Schwidefsky, Heiko Carstens, linux-s390, linux-kernel, kernel-janitors Hi Dan, thanks for reporting this netiucv-problem. There is an implication: Without fix, buffer values between 2**31 and 2**32 are not detected as invalid values. Your fix would help, but since we have to touch the code, I suggest to modernize it, moving from simple_strtoul() to kstrtouint(). This would be the patch I have in mind: --- a/drivers/s390/net/netiucv.c +++ b/drivers/s390/net/netiucv.c @@ -1564,21 +1564,21 @@ static ssize_t buffer_write (struct devi { struct netiucv_priv *priv = dev_get_drvdata(dev); struct net_device *ndev = priv->conn->netdev; - char *e; - int bs1; + unsigned int bs1; + int rc; IUCV_DBF_TEXT(trace, 3, __func__); if (count >= 39) return -EINVAL; - bs1 = simple_strtoul(buf, &e, 0); + rc = kstrtouint(buf, 0, &bs1); - if (e && (!isspace(*e))) { - IUCV_DBF_TEXT_(setup, 2, "buffer_write: invalid char %02x\n", - *e); + if (rc = -EINVAL) { + IUCV_DBF_TEXT_(setup, 2, "buffer_write: invalid char %s\n", + buf); return -EINVAL; } - if (bs1 > NETIUCV_BUFSIZE_MAX) { + if ((rc = -ERANGE) || (bs1 > NETIUCV_BUFSIZE_MAX)) { IUCV_DBF_TEXT_(setup, 2, "buffer_write: buffer size %d too large\n", bs1); And a question about your static checker detecting this problem: Is it a private checker, or something we could use as well? Does your checker like my patch version? Thanks, Ursula On 07/14/2016 01:30 PM, Dan Carpenter wrote: > I haven't looked at the implications but we accidentally allow bs1 to > be negative. It makes my static checker complain. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > diff --git a/drivers/s390/net/netiucv.c b/drivers/s390/net/netiucv.c > index b0e8ffd..85a5744 100644 > --- a/drivers/s390/net/netiucv.c > +++ b/drivers/s390/net/netiucv.c > @@ -1578,7 +1578,7 @@ static ssize_t buffer_write (struct device *dev, struct device_attribute *attr, > *e); > return -EINVAL; > } > - if (bs1 > NETIUCV_BUFSIZE_MAX) { > + if (bs1 < 0 || bs1 > NETIUCV_BUFSIZE_MAX) { > IUCV_DBF_TEXT_(setup, 2, > "buffer_write: buffer size %d too large\n", > bs1); > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] netiucv: silence an underflow warning @ 2016-07-15 7:11 ` Ursula Braun 0 siblings, 0 replies; 6+ messages in thread From: Ursula Braun @ 2016-07-15 7:11 UTC (permalink / raw) To: Dan Carpenter Cc: Martin Schwidefsky, Heiko Carstens, linux-s390, linux-kernel, kernel-janitors Hi Dan, thanks for reporting this netiucv-problem. There is an implication: Without fix, buffer values between 2**31 and 2**32 are not detected as invalid values. Your fix would help, but since we have to touch the code, I suggest to modernize it, moving from simple_strtoul() to kstrtouint(). This would be the patch I have in mind: --- a/drivers/s390/net/netiucv.c +++ b/drivers/s390/net/netiucv.c @@ -1564,21 +1564,21 @@ static ssize_t buffer_write (struct devi { struct netiucv_priv *priv = dev_get_drvdata(dev); struct net_device *ndev = priv->conn->netdev; - char *e; - int bs1; + unsigned int bs1; + int rc; IUCV_DBF_TEXT(trace, 3, __func__); if (count >= 39) return -EINVAL; - bs1 = simple_strtoul(buf, &e, 0); + rc = kstrtouint(buf, 0, &bs1); - if (e && (!isspace(*e))) { - IUCV_DBF_TEXT_(setup, 2, "buffer_write: invalid char %02x\n", - *e); + if (rc == -EINVAL) { + IUCV_DBF_TEXT_(setup, 2, "buffer_write: invalid char %s\n", + buf); return -EINVAL; } - if (bs1 > NETIUCV_BUFSIZE_MAX) { + if ((rc == -ERANGE) || (bs1 > NETIUCV_BUFSIZE_MAX)) { IUCV_DBF_TEXT_(setup, 2, "buffer_write: buffer size %d too large\n", bs1); And a question about your static checker detecting this problem: Is it a private checker, or something we could use as well? Does your checker like my patch version? Thanks, Ursula On 07/14/2016 01:30 PM, Dan Carpenter wrote: > I haven't looked at the implications but we accidentally allow bs1 to > be negative. It makes my static checker complain. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > diff --git a/drivers/s390/net/netiucv.c b/drivers/s390/net/netiucv.c > index b0e8ffd..85a5744 100644 > --- a/drivers/s390/net/netiucv.c > +++ b/drivers/s390/net/netiucv.c > @@ -1578,7 +1578,7 @@ static ssize_t buffer_write (struct device *dev, struct device_attribute *attr, > *e); > return -EINVAL; > } > - if (bs1 > NETIUCV_BUFSIZE_MAX) { > + if (bs1 < 0 || bs1 > NETIUCV_BUFSIZE_MAX) { > IUCV_DBF_TEXT_(setup, 2, > "buffer_write: buffer size %d too large\n", > bs1); > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] netiucv: silence an underflow warning 2016-07-15 7:11 ` Ursula Braun @ 2016-07-15 8:33 ` Dan Carpenter -1 siblings, 0 replies; 6+ messages in thread From: Dan Carpenter @ 2016-07-15 8:33 UTC (permalink / raw) To: Ursula Braun Cc: Martin Schwidefsky, Heiko Carstens, linux-s390, linux-kernel, kernel-janitors That looks good. Thanks. This was a Smatch thing that I haven't pushed yet. It needs some work yet to cut down the false positive rate but it's close. regards, dan carpenter ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [patch] netiucv: silence an underflow warning @ 2016-07-15 8:33 ` Dan Carpenter 0 siblings, 0 replies; 6+ messages in thread From: Dan Carpenter @ 2016-07-15 8:33 UTC (permalink / raw) To: Ursula Braun Cc: Martin Schwidefsky, Heiko Carstens, linux-s390, linux-kernel, kernel-janitors That looks good. Thanks. This was a Smatch thing that I haven't pushed yet. It needs some work yet to cut down the false positive rate but it's close. regards, dan carpenter ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-07-15 8:33 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-07-14 11:30 [patch] netiucv: silence an underflow warning Dan Carpenter 2016-07-14 11:30 ` Dan Carpenter 2016-07-15 7:11 ` Ursula Braun 2016-07-15 7:11 ` Ursula Braun 2016-07-15 8:33 ` Dan Carpenter 2016-07-15 8:33 ` Dan Carpenter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.