[PATCH] mei: bus: type promotion bug in mei_nfc_if_version()

[PATCH] mei: bus: type promotion bug in mei_nfc_if_version()

[Dan Carpenter]

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;

Re: [PATCH] mei: bus: type promotion bug in mei_nfc_if_version()

[Julia Lawall]

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
>

Re: [PATCH] mei: bus: type promotion bug in mei_nfc_if_version()

[Dan Carpenter]

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

RE: [PATCH] mei: bus: type promotion bug in mei_nfc_if_version()

[Winkler, Tomas]

> 
> 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

Re: [PATCH] mei: bus: type promotion bug in mei_nfc_if_version()

[Dan Carpenter]

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

RE: [PATCH] mei: bus: type promotion bug in mei_nfc_if_version()

[Winkler, Tomas]

> 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

RE: [PATCH] mei: bus: type promotion bug in mei_nfc_if_version()

[Julia Lawall]

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
>
>

Re: [PATCH] mei: bus: type promotion bug in mei_nfc_if_version()

[Greg Kroah-Hartman]

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 :(

RE: [PATCH] mei: bus: type promotion bug in mei_nfc_if_version()

[Winkler, Tomas]

> 
> 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

Re: [PATCH] mei: bus: type promotion bug in mei_nfc_if_version()

[Greg Kroah-Hartman]

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