* [PATCH] mei: bus: type promotion bug in mei_nfc_if_version() @ 2018-07-04 9:34 Dan Carpenter 2018-07-04 11:59 ` Julia Lawall 2018-07-07 15:32 ` Greg Kroah-Hartman 0 siblings, 2 replies; 10+ messages in thread From: Dan Carpenter @ 2018-07-04 9:34 UTC (permalink / raw) To: Tomas Winkler, Alexander Usyskin Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-kernel, kernel-janitors We accidentally removed the check for negative returns without considering the issue of type promotion. The "if_version_length" variable is type size_t so if __mei_cl_recv() returns a negative then "bytes_recv" is type promoted to a high positive value and treated as success. Fixes: 582ab27a063a ("mei: bus: fix received data size check in NFC fixup") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> diff --git a/drivers/misc/mei/bus-fixup.c b/drivers/misc/mei/bus-fixup.c index 0208c4b027c5..fa0236a5e59a 100644 --- a/drivers/misc/mei/bus-fixup.c +++ b/drivers/misc/mei/bus-fixup.c @@ -267,7 +267,7 @@ static int mei_nfc_if_version(struct mei_cl *cl, ret = 0; bytes_recv = __mei_cl_recv(cl, (u8 *)reply, if_version_length, 0); - if (bytes_recv < if_version_length) { + if (bytes_recv < 0 || bytes_recv < if_version_length) { dev_err(bus->dev, "Could not read IF version\n"); ret = -EIO; goto err; ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] mei: bus: type promotion bug in mei_nfc_if_version() 2018-07-04 9:34 [PATCH] mei: bus: type promotion bug in mei_nfc_if_version() Dan Carpenter @ 2018-07-04 11:59 ` Julia Lawall 2018-07-04 12:16 ` Dan Carpenter 2018-07-07 15:32 ` Greg Kroah-Hartman 1 sibling, 1 reply; 10+ messages in thread From: Julia Lawall @ 2018-07-04 11:59 UTC (permalink / raw) To: Dan Carpenter Cc: Tomas Winkler, Alexander Usyskin, Arnd Bergmann, Greg Kroah-Hartman, linux-kernel, kernel-janitors On Wed, 4 Jul 2018, Dan Carpenter wrote: > We accidentally removed the check for negative returns without > considering the issue of type promotion. The "if_version_length" > variable is type size_t so if __mei_cl_recv() returns a negative then > "bytes_recv" is type promoted to a high positive value and treated as > success. > > Fixes: 582ab27a063a ("mei: bus: fix received data size check in NFC fixup") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > diff --git a/drivers/misc/mei/bus-fixup.c b/drivers/misc/mei/bus-fixup.c > index 0208c4b027c5..fa0236a5e59a 100644 > --- a/drivers/misc/mei/bus-fixup.c > +++ b/drivers/misc/mei/bus-fixup.c > @@ -267,7 +267,7 @@ static int mei_nfc_if_version(struct mei_cl *cl, > > ret = 0; > bytes_recv = __mei_cl_recv(cl, (u8 *)reply, if_version_length, 0); > - if (bytes_recv < if_version_length) { > + if (bytes_recv < 0 || bytes_recv < if_version_length) { Is this preferred to adding an int cast? julia > dev_err(bus->dev, "Could not read IF version\n"); > ret = -EIO; > goto err; > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mei: bus: type promotion bug in mei_nfc_if_version() 2018-07-04 11:59 ` Julia Lawall @ 2018-07-04 12:16 ` Dan Carpenter 2018-07-04 13:57 ` Winkler, Tomas 0 siblings, 1 reply; 10+ messages in thread From: Dan Carpenter @ 2018-07-04 12:16 UTC (permalink / raw) To: Julia Lawall Cc: Tomas Winkler, Alexander Usyskin, Arnd Bergmann, Greg Kroah-Hartman, linux-kernel, kernel-janitors On Wed, Jul 04, 2018 at 01:59:14PM +0200, Julia Lawall wrote: > > > On Wed, 4 Jul 2018, Dan Carpenter wrote: > > > We accidentally removed the check for negative returns without > > considering the issue of type promotion. The "if_version_length" > > variable is type size_t so if __mei_cl_recv() returns a negative then > > "bytes_recv" is type promoted to a high positive value and treated as > > success. > > > > Fixes: 582ab27a063a ("mei: bus: fix received data size check in NFC fixup") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > diff --git a/drivers/misc/mei/bus-fixup.c b/drivers/misc/mei/bus-fixup.c > > index 0208c4b027c5..fa0236a5e59a 100644 > > --- a/drivers/misc/mei/bus-fixup.c > > +++ b/drivers/misc/mei/bus-fixup.c > > @@ -267,7 +267,7 @@ static int mei_nfc_if_version(struct mei_cl *cl, > > > > ret = 0; > > bytes_recv = __mei_cl_recv(cl, (u8 *)reply, if_version_length, 0); > > - if (bytes_recv < if_version_length) { > > + if (bytes_recv < 0 || bytes_recv < if_version_length) { > > Is this preferred to adding an int cast? I don't think it matters. I kind of like explicitly testing for negative but maybe later people will just remove the check like we did here? You could do it a bunch of different ways: 1: if (ret < 0 || ret < ARRAY_SIZE(xxx)) 2: if (ret < (int)ARRAY_SIZE(xxx)) 3: if (ret != ARRAY_SIZE(xxx)) They're all equivalent. I guess I don't like casting too much. My first approach to fixing this was just to declare if_version_length as an int, but then I saw that originally there was a "bytes_recv < 0" check and decided to go that way instead. regards, dan carpenter ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] mei: bus: type promotion bug in mei_nfc_if_version() 2018-07-04 12:16 ` Dan Carpenter @ 2018-07-04 13:57 ` Winkler, Tomas 2018-07-04 14:15 ` Dan Carpenter 0 siblings, 1 reply; 10+ messages in thread From: Winkler, Tomas @ 2018-07-04 13:57 UTC (permalink / raw) To: Dan Carpenter, Julia Lawall Cc: Usyskin, Alexander, Arnd Bergmann, Greg Kroah-Hartman, linux-kernel, kernel-janitors > > On Wed, Jul 04, 2018 at 01:59:14PM +0200, Julia Lawall wrote: > > > > > > On Wed, 4 Jul 2018, Dan Carpenter wrote: > > > > > We accidentally removed the check for negative returns without > > > considering the issue of type promotion. The "if_version_length" > > > variable is type size_t so if __mei_cl_recv() returns a negative > > > then "bytes_recv" is type promoted to a high positive value and > > > treated as success. > > > > > > Fixes: 582ab27a063a ("mei: bus: fix received data size check in NFC > > > fixup") > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > > > diff --git a/drivers/misc/mei/bus-fixup.c > > > b/drivers/misc/mei/bus-fixup.c index 0208c4b027c5..fa0236a5e59a > > > 100644 > > > --- a/drivers/misc/mei/bus-fixup.c > > > +++ b/drivers/misc/mei/bus-fixup.c > > > @@ -267,7 +267,7 @@ static int mei_nfc_if_version(struct mei_cl *cl, > > > > > > ret = 0; > > > bytes_recv = __mei_cl_recv(cl, (u8 *)reply, if_version_length, 0); > > > - if (bytes_recv < if_version_length) { > > > + if (bytes_recv < 0 || bytes_recv < if_version_length) { > > > > Is this preferred to adding an int cast? > > I don't think it matters. I kind of like explicitly testing for negative but > maybe later people will just remove the check like we did here? You could > do it a bunch of different ways: > > 1: if (ret < 0 || ret < ARRAY_SIZE(xxx)) > 2: if (ret < (int)ARRAY_SIZE(xxx)) > 3: if (ret != ARRAY_SIZE(xxx)) > > They're all equivalent. I guess I don't like casting too much. My first > approach to fixing this was just to declare if_version_length as an int, but > then I saw that originally there was a "bytes_recv < 0" > check and decided to go that way instead. Actually bytes_recv should be probably of ssize_t type, so could be the if_version_length. How did you find this, I haven't seen it in reported by sparse, smatch and I believe -Wsign-compare is suppressed in compilation warnings. Thanks Tomas ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mei: bus: type promotion bug in mei_nfc_if_version() 2018-07-04 13:57 ` Winkler, Tomas @ 2018-07-04 14:15 ` Dan Carpenter 2018-07-04 14:25 ` Winkler, Tomas 0 siblings, 1 reply; 10+ messages in thread From: Dan Carpenter @ 2018-07-04 14:15 UTC (permalink / raw) To: Winkler, Tomas Cc: Julia Lawall, Usyskin, Alexander, Arnd Bergmann, Greg Kroah-Hartman, linux-kernel, kernel-janitors On Wed, Jul 04, 2018 at 01:57:44PM +0000, Winkler, Tomas wrote: > > > > On Wed, Jul 04, 2018 at 01:59:14PM +0200, Julia Lawall wrote: > > > > > > > > > On Wed, 4 Jul 2018, Dan Carpenter wrote: > > > > > > > We accidentally removed the check for negative returns without > > > > considering the issue of type promotion. The "if_version_length" > > > > variable is type size_t so if __mei_cl_recv() returns a negative > > > > then "bytes_recv" is type promoted to a high positive value and > > > > treated as success. > > > > > > > > Fixes: 582ab27a063a ("mei: bus: fix received data size check in NFC > > > > fixup") > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > > > > > diff --git a/drivers/misc/mei/bus-fixup.c > > > > b/drivers/misc/mei/bus-fixup.c index 0208c4b027c5..fa0236a5e59a > > > > 100644 > > > > --- a/drivers/misc/mei/bus-fixup.c > > > > +++ b/drivers/misc/mei/bus-fixup.c > > > > @@ -267,7 +267,7 @@ static int mei_nfc_if_version(struct mei_cl *cl, > > > > > > > > ret = 0; > > > > bytes_recv = __mei_cl_recv(cl, (u8 *)reply, if_version_length, 0); > > > > - if (bytes_recv < if_version_length) { > > > > + if (bytes_recv < 0 || bytes_recv < if_version_length) { > > > > > > Is this preferred to adding an int cast? > > > > I don't think it matters. I kind of like explicitly testing for negative but > > maybe later people will just remove the check like we did here? You could > > do it a bunch of different ways: > > > > 1: if (ret < 0 || ret < ARRAY_SIZE(xxx)) > > 2: if (ret < (int)ARRAY_SIZE(xxx)) > > 3: if (ret != ARRAY_SIZE(xxx)) > > > > They're all equivalent. I guess I don't like casting too much. My first > > approach to fixing this was just to declare if_version_length as an int, but > > then I saw that originally there was a "bytes_recv < 0" > > check and decided to go that way instead. > > Actually bytes_recv should be probably of ssize_t type, so could be the if_version_length. > > How did you find this, I haven't seen it in reported by sparse, smatch and I believe -Wsign-compare is suppressed in compilation warnings. It's a new thing. Julia noticed this kind of bug first and I have been mucking around with it in Smatch as well. My Smatch check has too many false positives to publish right now because it thinks a some common functions like ffs() return negative error codes. regards, dan carpenter ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] mei: bus: type promotion bug in mei_nfc_if_version() 2018-07-04 14:15 ` Dan Carpenter @ 2018-07-04 14:25 ` Winkler, Tomas 2018-07-04 14:45 ` Julia Lawall 0 siblings, 1 reply; 10+ messages in thread From: Winkler, Tomas @ 2018-07-04 14:25 UTC (permalink / raw) To: Dan Carpenter Cc: Julia Lawall, Usyskin, Alexander, Arnd Bergmann, Greg Kroah-Hartman, linux-kernel, kernel-janitors > On Wed, Jul 04, 2018 at 01:57:44PM +0000, Winkler, Tomas wrote: > > > > > > On Wed, Jul 04, 2018 at 01:59:14PM +0200, Julia Lawall wrote: > > > > > > > > > > > > On Wed, 4 Jul 2018, Dan Carpenter wrote: > > > > > > > > > We accidentally removed the check for negative returns without > > > > > considering the issue of type promotion. The "if_version_length" > > > > > variable is type size_t so if __mei_cl_recv() returns a negative > > > > > then "bytes_recv" is type promoted to a high positive value and > > > > > treated as success. > > > > > > > > > > Fixes: 582ab27a063a ("mei: bus: fix received data size check in > > > > > NFC > > > > > fixup") > > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > > > > > > > diff --git a/drivers/misc/mei/bus-fixup.c > > > > > b/drivers/misc/mei/bus-fixup.c index 0208c4b027c5..fa0236a5e59a > > > > > 100644 > > > > > --- a/drivers/misc/mei/bus-fixup.c > > > > > +++ b/drivers/misc/mei/bus-fixup.c > > > > > @@ -267,7 +267,7 @@ static int mei_nfc_if_version(struct mei_cl > > > > > *cl, > > > > > > > > > > ret = 0; > > > > > bytes_recv = __mei_cl_recv(cl, (u8 *)reply, if_version_length, > 0); > > > > > - if (bytes_recv < if_version_length) { > > > > > + if (bytes_recv < 0 || bytes_recv < if_version_length) { > > > > > > > > Is this preferred to adding an int cast? > > > > > > I don't think it matters. I kind of like explicitly testing for > > > negative but maybe later people will just remove the check like we > > > did here? You could do it a bunch of different ways: > > > > > > 1: if (ret < 0 || ret < ARRAY_SIZE(xxx)) > > > 2: if (ret < (int)ARRAY_SIZE(xxx)) > > > 3: if (ret != ARRAY_SIZE(xxx)) > > > > > > They're all equivalent. I guess I don't like casting too much. My > > > first approach to fixing this was just to declare if_version_length > > > as an int, but then I saw that originally there was a "bytes_recv < 0" > > > check and decided to go that way instead. > > > > Actually bytes_recv should be probably of ssize_t type, so could be the > if_version_length. > > > > How did you find this, I haven't seen it in reported by sparse, smatch and I > believe -Wsign-compare is suppressed in compilation warnings. > > It's a new thing. Julia noticed this kind of bug first and I have been mucking > around with it in Smatch as well. My Smatch check has too many false > positives to publish right now because it thinks a some common functions > like ffs() return negative error codes. I guess this is why it is suppressed in the compilation warnings in the first place. Maybe need to disable it selectively, like for fss, just not sure how bad is that with false positive reports. Thanks Tomas ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] mei: bus: type promotion bug in mei_nfc_if_version() 2018-07-04 14:25 ` Winkler, Tomas @ 2018-07-04 14:45 ` Julia Lawall 0 siblings, 0 replies; 10+ messages in thread From: Julia Lawall @ 2018-07-04 14:45 UTC (permalink / raw) To: Winkler, Tomas Cc: Dan Carpenter, Usyskin, Alexander, Arnd Bergmann, Greg Kroah-Hartman, linux-kernel, kernel-janitors On Wed, 4 Jul 2018, Winkler, Tomas wrote: > > > On Wed, Jul 04, 2018 at 01:57:44PM +0000, Winkler, Tomas wrote: > > > > > > > > On Wed, Jul 04, 2018 at 01:59:14PM +0200, Julia Lawall wrote: > > > > > > > > > > > > > > > On Wed, 4 Jul 2018, Dan Carpenter wrote: > > > > > > > > > > > We accidentally removed the check for negative returns without > > > > > > considering the issue of type promotion. The "if_version_length" > > > > > > variable is type size_t so if __mei_cl_recv() returns a negative > > > > > > then "bytes_recv" is type promoted to a high positive value and > > > > > > treated as success. > > > > > > > > > > > > Fixes: 582ab27a063a ("mei: bus: fix received data size check in > > > > > > NFC > > > > > > fixup") > > > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > > > > > > > > > diff --git a/drivers/misc/mei/bus-fixup.c > > > > > > b/drivers/misc/mei/bus-fixup.c index 0208c4b027c5..fa0236a5e59a > > > > > > 100644 > > > > > > --- a/drivers/misc/mei/bus-fixup.c > > > > > > +++ b/drivers/misc/mei/bus-fixup.c > > > > > > @@ -267,7 +267,7 @@ static int mei_nfc_if_version(struct mei_cl > > > > > > *cl, > > > > > > > > > > > > ret = 0; > > > > > > bytes_recv = __mei_cl_recv(cl, (u8 *)reply, if_version_length, > > 0); > > > > > > - if (bytes_recv < if_version_length) { > > > > > > + if (bytes_recv < 0 || bytes_recv < if_version_length) { > > > > > > > > > > Is this preferred to adding an int cast? > > > > > > > > I don't think it matters. I kind of like explicitly testing for > > > > negative but maybe later people will just remove the check like we > > > > did here? You could do it a bunch of different ways: > > > > > > > > 1: if (ret < 0 || ret < ARRAY_SIZE(xxx)) > > > > 2: if (ret < (int)ARRAY_SIZE(xxx)) > > > > 3: if (ret != ARRAY_SIZE(xxx)) > > > > > > > > They're all equivalent. I guess I don't like casting too much. My > > > > first approach to fixing this was just to declare if_version_length > > > > as an int, but then I saw that originally there was a "bytes_recv < 0" > > > > check and decided to go that way instead. > > > > > > Actually bytes_recv should be probably of ssize_t type, so could be the > > if_version_length. > > > > > > How did you find this, I haven't seen it in reported by sparse, smatch and I > > believe -Wsign-compare is suppressed in compilation warnings. > > > > It's a new thing. Julia noticed this kind of bug first Actually, I got it from Joe Perches, for the sizeof case :) julia > > and I have been mucking > > around with it in Smatch as well. My Smatch check has too many false > > positives to publish right now because it thinks a some common functions > > like ffs() return negative error codes. > > I guess this is why it is suppressed in the compilation warnings in the first place. > Maybe need to disable it selectively, like for fss, just not sure how bad is that with false positive reports. > > > Thanks > Tomas > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mei: bus: type promotion bug in mei_nfc_if_version() 2018-07-04 9:34 [PATCH] mei: bus: type promotion bug in mei_nfc_if_version() Dan Carpenter 2018-07-04 11:59 ` Julia Lawall @ 2018-07-07 15:32 ` Greg Kroah-Hartman 2018-07-09 11:36 ` Winkler, Tomas 1 sibling, 1 reply; 10+ messages in thread From: Greg Kroah-Hartman @ 2018-07-07 15:32 UTC (permalink / raw) To: Dan Carpenter Cc: Tomas Winkler, Alexander Usyskin, Arnd Bergmann, linux-kernel, kernel-janitors On Wed, Jul 04, 2018 at 12:34:49PM +0300, Dan Carpenter wrote: > We accidentally removed the check for negative returns without > considering the issue of type promotion. The "if_version_length" > variable is type size_t so if __mei_cl_recv() returns a negative then > "bytes_recv" is type promoted to a high positive value and treated as > success. > > Fixes: 582ab27a063a ("mei: bus: fix received data size check in NFC fixup") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Didn't apply to my tree at all :( ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] mei: bus: type promotion bug in mei_nfc_if_version() 2018-07-07 15:32 ` Greg Kroah-Hartman @ 2018-07-09 11:36 ` Winkler, Tomas 2018-07-09 12:03 ` Greg Kroah-Hartman 0 siblings, 1 reply; 10+ messages in thread From: Winkler, Tomas @ 2018-07-09 11:36 UTC (permalink / raw) To: Greg Kroah-Hartman, Dan Carpenter Cc: Usyskin, Alexander, Arnd Bergmann, linux-kernel, kernel-janitors > > On Wed, Jul 04, 2018 at 12:34:49PM +0300, Dan Carpenter wrote: > > We accidentally removed the check for negative returns without > > considering the issue of type promotion. The "if_version_length" > > variable is type size_t so if __mei_cl_recv() returns a negative then > > "bytes_recv" is type promoted to a high positive value and treated as > > success. > > > > Fixes: 582ab27a063a ("mei: bus: fix received data size check in NFC > > fixup") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > Didn't apply to my tree at all :( I will rework this, anyhow I would prefer if you can wait for my signoff before applying, I believe we are reasonably responsive. Thanks Tomas ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] mei: bus: type promotion bug in mei_nfc_if_version() 2018-07-09 11:36 ` Winkler, Tomas @ 2018-07-09 12:03 ` Greg Kroah-Hartman 0 siblings, 0 replies; 10+ messages in thread From: Greg Kroah-Hartman @ 2018-07-09 12:03 UTC (permalink / raw) To: Winkler, Tomas Cc: Dan Carpenter, Usyskin, Alexander, Arnd Bergmann, linux-kernel, kernel-janitors On Mon, Jul 09, 2018 at 11:36:21AM +0000, Winkler, Tomas wrote: > > > > On Wed, Jul 04, 2018 at 12:34:49PM +0300, Dan Carpenter wrote: > > > We accidentally removed the check for negative returns without > > > considering the issue of type promotion. The "if_version_length" > > > variable is type size_t so if __mei_cl_recv() returns a negative then > > > "bytes_recv" is type promoted to a high positive value and treated as > > > success. > > > > > > Fixes: 582ab27a063a ("mei: bus: fix received data size check in NFC > > > fixup") > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > Didn't apply to my tree at all :( > > I will rework this, anyhow I would prefer if you can wait for my signoff before applying, > I believe we are reasonably responsive. Yes you are, but for trusted developers, it's fine to merge their stuff as well. Remember, maintainers are not "gatekeepers" :) thanks, greg k-h ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-07-09 12:03 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-04 9:34 [PATCH] mei: bus: type promotion bug in mei_nfc_if_version() Dan Carpenter 2018-07-04 11:59 ` Julia Lawall 2018-07-04 12:16 ` Dan Carpenter 2018-07-04 13:57 ` Winkler, Tomas 2018-07-04 14:15 ` Dan Carpenter 2018-07-04 14:25 ` Winkler, Tomas 2018-07-04 14:45 ` Julia Lawall 2018-07-07 15:32 ` Greg Kroah-Hartman 2018-07-09 11:36 ` Winkler, Tomas 2018-07-09 12:03 ` Greg Kroah-Hartman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).