* [PATCH] scsi: aic7xxx: Fix unintended sign extension issue
@ 2018-10-25 15:13 ` Colin King
0 siblings, 0 replies; 10+ messages in thread
From: Colin King @ 2018-10-25 15:13 UTC (permalink / raw)
To: Hannes Reinecke, James E . J . Bottomley, Martin K . Petersen,
linux-scsi
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
In the expression "ahc_inb(ahc, port+3) << 24", the initial value is a
u8, but is promoted to a signed int, then sign-extended to uint64_t. If
the value read from the port has the upper bit set then the sign
extension will set all the upper bits of the expression which is probably
not what was intended. Cast to uint64_t to avoid the sign extension.
Detected by CoverityScan, CID#138806, 138807 ("Unintended sign extension")
Fixes: be0d67680d52 ("[SCSI] aic7xxx, aic79xx: deinline functions")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/scsi/aic7xxx/aic79xx_core.c | 2 +-
drivers/scsi/aic7xxx/aic7xxx_core.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/aic7xxx/aic79xx_core.c b/drivers/scsi/aic7xxx/aic79xx_core.c
index 9ee75c9a9aa1..a836233edb91 100644
--- a/drivers/scsi/aic7xxx/aic79xx_core.c
+++ b/drivers/scsi/aic7xxx/aic79xx_core.c
@@ -622,7 +622,7 @@ ahd_inq(struct ahd_softc *ahd, u_int port)
return ((ahd_inb(ahd, port))
| (ahd_inb(ahd, port+1) << 8)
| (ahd_inb(ahd, port+2) << 16)
- | (ahd_inb(ahd, port+3) << 24)
+ | (((uint64_t)ahd_inb(ahd, port+3)) << 24)
| (((uint64_t)ahd_inb(ahd, port+4)) << 32)
| (((uint64_t)ahd_inb(ahd, port+5)) << 40)
| (((uint64_t)ahd_inb(ahd, port+6)) << 48)
diff --git a/drivers/scsi/aic7xxx/aic7xxx_core.c b/drivers/scsi/aic7xxx/aic7xxx_core.c
index f3362f4ab16e..74d3f1dd0427 100644
--- a/drivers/scsi/aic7xxx/aic7xxx_core.c
+++ b/drivers/scsi/aic7xxx/aic7xxx_core.c
@@ -493,7 +493,7 @@ ahc_inq(struct ahc_softc *ahc, u_int port)
return ((ahc_inb(ahc, port))
| (ahc_inb(ahc, port+1) << 8)
| (ahc_inb(ahc, port+2) << 16)
- | (ahc_inb(ahc, port+3) << 24)
+ | (((uint64_t)ahc_inb(ahc, port+3)) << 24)
| (((uint64_t)ahc_inb(ahc, port+4)) << 32)
| (((uint64_t)ahc_inb(ahc, port+5)) << 40)
| (((uint64_t)ahc_inb(ahc, port+6)) << 48)
--
2.19.1
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH] scsi: aic7xxx: Fix unintended sign extension issue
2018-10-25 15:13 ` Colin King
@ 2018-10-25 15:32 ` Joe Perches
-1 siblings, 0 replies; 10+ messages in thread
From: Joe Perches @ 2018-10-25 15:32 UTC (permalink / raw)
To: Colin King, Hannes Reinecke, James E . J . Bottomley,
Martin K . Petersen, linux-scsi
Cc: kernel-janitors, linux-kernel
On Thu, 2018-10-25 at 16:13 +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> In the expression "ahc_inb(ahc, port+3) << 24", the initial value is a
> u8, but is promoted to a signed int, then sign-extended to uint64_t. If
> the value read from the port has the upper bit set then the sign
> extension will set all the upper bits of the expression which is probably
> not what was intended. Cast to uint64_t to avoid the sign extension.
[]
> diff --git a/drivers/scsi/aic7xxx/aic79xx_core.c b/drivers/scsi/aic7xxx/aic79xx_core.c
[]
> @@ -622,7 +622,7 @@ ahd_inq(struct ahd_softc *ahd, u_int port)
> return ((ahd_inb(ahd, port))
> | (ahd_inb(ahd, port+1) << 8)
> | (ahd_inb(ahd, port+2) << 16)
> - | (ahd_inb(ahd, port+3) << 24)
> + | (((uint64_t)ahd_inb(ahd, port+3)) << 24)
> | (((uint64_t)ahd_inb(ahd, port+4)) << 32)
> | (((uint64_t)ahd_inb(ahd, port+5)) << 40)
> | (((uint64_t)ahd_inb(ahd, port+6)) << 48)
Perhaps a different method using two calls to ahd_inl
is clearer and possibly faster like:
uint64_t
ahd_inq(struct ahd_softc *ahd, u_int port)
{
return ahd_inl(port) | ((uint64_t)ahd_inl(port + 4) << 32);
}
> diff --git a/drivers/scsi/aic7xxx/aic7xxx_core.c b/drivers/scsi/aic7xxx/aic7xxx_core.c
[]
> @@ -493,7 +493,7 @@ ahc_inq(struct ahc_softc *ahc, u_int port)
> return ((ahc_inb(ahc, port))
> | (ahc_inb(ahc, port+1) << 8)
> | (ahc_inb(ahc, port+2) << 16)
> - | (ahc_inb(ahc, port+3) << 24)
> + | (((uint64_t)ahc_inb(ahc, port+3)) << 24)
> | (((uint64_t)ahc_inb(ahc, port+4)) << 32)
> | (((uint64_t)ahc_inb(ahc, port+5)) << 40)
> | (((uint64_t)ahc_inb(ahc, port+6)) << 48)
here too
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] scsi: aic7xxx: Fix unintended sign extension issue
@ 2018-10-25 15:32 ` Joe Perches
0 siblings, 0 replies; 10+ messages in thread
From: Joe Perches @ 2018-10-25 15:32 UTC (permalink / raw)
To: Colin King, Hannes Reinecke, James E . J . Bottomley,
Martin K . Petersen, linux-scsi
Cc: kernel-janitors, linux-kernel
On Thu, 2018-10-25 at 16:13 +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> In the expression "ahc_inb(ahc, port+3) << 24", the initial value is a
> u8, but is promoted to a signed int, then sign-extended to uint64_t. If
> the value read from the port has the upper bit set then the sign
> extension will set all the upper bits of the expression which is probably
> not what was intended. Cast to uint64_t to avoid the sign extension.
[]
> diff --git a/drivers/scsi/aic7xxx/aic79xx_core.c b/drivers/scsi/aic7xxx/aic79xx_core.c
[]
> @@ -622,7 +622,7 @@ ahd_inq(struct ahd_softc *ahd, u_int port)
> return ((ahd_inb(ahd, port))
> | (ahd_inb(ahd, port+1) << 8)
> | (ahd_inb(ahd, port+2) << 16)
> - | (ahd_inb(ahd, port+3) << 24)
> + | (((uint64_t)ahd_inb(ahd, port+3)) << 24)
> | (((uint64_t)ahd_inb(ahd, port+4)) << 32)
> | (((uint64_t)ahd_inb(ahd, port+5)) << 40)
> | (((uint64_t)ahd_inb(ahd, port+6)) << 48)
Perhaps a different method using two calls to ahd_inl
is clearer and possibly faster like:
uint64_t
ahd_inq(struct ahd_softc *ahd, u_int port)
{
return ahd_inl(port) | ((uint64_t)ahd_inl(port + 4) << 32);
}
> diff --git a/drivers/scsi/aic7xxx/aic7xxx_core.c b/drivers/scsi/aic7xxx/aic7xxx_core.c
[]
> @@ -493,7 +493,7 @@ ahc_inq(struct ahc_softc *ahc, u_int port)
> return ((ahc_inb(ahc, port))
> | (ahc_inb(ahc, port+1) << 8)
> | (ahc_inb(ahc, port+2) << 16)
> - | (ahc_inb(ahc, port+3) << 24)
> + | (((uint64_t)ahc_inb(ahc, port+3)) << 24)
> | (((uint64_t)ahc_inb(ahc, port+4)) << 32)
> | (((uint64_t)ahc_inb(ahc, port+5)) << 40)
> | (((uint64_t)ahc_inb(ahc, port+6)) << 48)
here too
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] scsi: aic7xxx: Fix unintended sign extension issue
2018-10-25 15:13 ` Colin King
@ 2018-10-25 15:32 ` James Bottomley
-1 siblings, 0 replies; 10+ messages in thread
From: James Bottomley @ 2018-10-25 15:32 UTC (permalink / raw)
To: Colin King, Hannes Reinecke, Martin K . Petersen, linux-scsi
Cc: kernel-janitors, linux-kernel
On Thu, 2018-10-25 at 16:13 +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> In the expression "ahc_inb(ahc, port+3) << 24", the initial value is
> a u8, but is promoted to a signed int, then sign-extended to
> uint64_t.
Why is this, that's highly non intuitive? The compiler is supposed to
promote to the biggest type, which is uint64_t and then do the
calculation
James
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] scsi: aic7xxx: Fix unintended sign extension issue
@ 2018-10-25 15:32 ` James Bottomley
0 siblings, 0 replies; 10+ messages in thread
From: James Bottomley @ 2018-10-25 15:32 UTC (permalink / raw)
To: Colin King, Hannes Reinecke, Martin K . Petersen, linux-scsi
Cc: kernel-janitors, linux-kernel
On Thu, 2018-10-25 at 16:13 +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> In the expression "ahc_inb(ahc, port+3) << 24", the initial value is
> a u8, but is promoted to a signed int, then sign-extended to
> uint64_t.
Why is this, that's highly non intuitive? The compiler is supposed to
promote to the biggest type, which is uint64_t and then do the
calculation
James
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] scsi: aic7xxx: Fix unintended sign extension issue
2018-10-25 15:32 ` James Bottomley
@ 2018-10-25 15:54 ` David Laight
-1 siblings, 0 replies; 10+ messages in thread
From: David Laight @ 2018-10-25 15:54 UTC (permalink / raw)
To: 'James Bottomley', Colin King, Hannes Reinecke,
Martin K . Petersen, linux-scsi@vger.kernel.org
Cc: kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org
RnJvbTogSmFtZXMgQm90dG9tbGV5DQo+IFNlbnQ6IDI1IE9jdG9iZXIgMjAxOCAxNjozMw0KPiAN
Cj4gT24gVGh1LCAyMDE4LTEwLTI1IGF0IDE2OjEzICswMTAwLCBDb2xpbiBLaW5nIHdyb3RlOg0K
PiA+IEZyb206IENvbGluIElhbiBLaW5nIDxjb2xpbi5raW5nQGNhbm9uaWNhbC5jb20+DQo+ID4N
Cj4gPiBJbiB0aGUgZXhwcmVzc2lvbiAiYWhjX2luYihhaGMsIHBvcnQrMykgPDwgMjQiLCB0aGUg
aW5pdGlhbCB2YWx1ZSBpcw0KPiA+IGEgdTgsIGJ1dCBpcyBwcm9tb3RlZCB0byBhIHNpZ25lZCBp
bnQsIHRoZW4gc2lnbi1leHRlbmRlZCB0bw0KPiA+IHVpbnQ2NF90Lg0KPiANCj4gV2h5IGlzIHRo
aXMsIHRoYXQncyBoaWdobHkgbm9uIGludHVpdGl2ZT8gIFRoZSBjb21waWxlciBpcyBzdXBwb3Nl
ZCB0bw0KPiBwcm9tb3RlIHRvIHRoZSBiaWdnZXN0IHR5cGUsIHdoaWNoIGlzIHVpbnQ2NF90IGFu
ZCB0aGVuIGRvIHRoZQ0KPiBjYWxjdWxhdGlvbg0KDQpEbyBub3QgZG91YnQgdGhlIHdpc2RvbSBv
biB0aGUgQU5TSSBDIGNvbW1pdHRlZSB0aGF0IGRlY2lkZWQgdG8gZG8NCid2YWx1ZSBwcmVzZXJ2
aW5nJyBpbnRlZ2VyIHByb21vdGlvbnMgaW5zdGVhZCBvZiB0aGUgJ3NpZ24gcHJlc2VydmluZycN
Cm9uZXMgb2YgSyZSIEMuDQoNClNvICd1bnNpZ25lZCBjaGFyJyBpcyBwcm9tb3RlZCB0byAnaW50
JyBhbG1vc3QgZXZlcnl3aGVyZSBpdCBpcyB1c2VkDQoodW5sZXNzIHRoZXkgYXJlIGJvdGggdGhl
IHNhbWUgc2l6ZSAtIHdoaWNoIGlzIGFsbG93ZWQpLg0KVGhpcyBtZWFucyB0aGF0IGFoY19pbmIo
KSA8PCAyNCBpcyBhY3R1YWxseSB1bmRlZmluZWQgKHNpZ25lZCBpbnRlZ2VyDQpvdmVyZmxvdyBj
YW4gZG8gYW55dGhpbmcgaXQgbGlrZXMpLg0KDQpCeSBmYXIgdGhlIGJlc3QgZml4IGlzIHRvIGNo
YW5nZSB0aGUgcmV0dXJuIHR5cGUgb2YgYWhjX2luYigpIHRvDQpiZSAndW5zaWduZWQgaW50Jy4N
Ck9uIHN5c3RlbXMgd2l0aG91dCBieXRlIHNpemVkIHJlZ2lzdGVycyAoYWJvdXQgZXZlcnl0aGlu
ZyBleGNlcHQgeDg2KQ0KdGhpcyB3aWxsIGFsbW9zdCBjZXJ0YWlubHkgZ2VuZXJhdGUgYmV0dGVy
IGNvZGUuDQoNCglEYXZpZA0KDQotDQpSZWdpc3RlcmVkIEFkZHJlc3MgTGFrZXNpZGUsIEJyYW1s
ZXkgUm9hZCwgTW91bnQgRmFybSwgTWlsdG9uIEtleW5lcywgTUsxIDFQVCwgVUsNClJlZ2lzdHJh
dGlvbiBObzogMTM5NzM4NiAoV2FsZXMpDQo
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] scsi: aic7xxx: Fix unintended sign extension issue
@ 2018-10-25 15:54 ` David Laight
0 siblings, 0 replies; 10+ messages in thread
From: David Laight @ 2018-10-25 15:54 UTC (permalink / raw)
To: 'James Bottomley', Colin King, Hannes Reinecke,
Martin K . Petersen, linux-scsi@vger.kernel.org
Cc: kernel-janitors@vger.kernel.org, linux-kernel@vger.kernel.org
From: James Bottomley
> Sent: 25 October 2018 16:33
>
> On Thu, 2018-10-25 at 16:13 +0100, Colin King wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> >
> > In the expression "ahc_inb(ahc, port+3) << 24", the initial value is
> > a u8, but is promoted to a signed int, then sign-extended to
> > uint64_t.
>
> Why is this, that's highly non intuitive? The compiler is supposed to
> promote to the biggest type, which is uint64_t and then do the
> calculation
Do not doubt the wisdom on the ANSI C committee that decided to do
'value preserving' integer promotions instead of the 'sign preserving'
ones of K&R C.
So 'unsigned char' is promoted to 'int' almost everywhere it is used
(unless they are both the same size - which is allowed).
This means that ahc_inb() << 24 is actually undefined (signed integer
overflow can do anything it likes).
By far the best fix is to change the return type of ahc_inb() to
be 'unsigned int'.
On systems without byte sized registers (about everything except x86)
this will almost certainly generate better code.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] scsi: aic7xxx: fix unintended sign extension on left shifts
@ 2019-10-14 12:56 ` Colin King
0 siblings, 0 replies; 10+ messages in thread
From: Colin King @ 2019-10-14 12:56 UTC (permalink / raw)
To: Hannes Reinecke, James E . J . Bottomley, Martin K . Petersen,
linux-scsi
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Shifting a u8 left will cause the value to be promoted to an integer. If
the top bit of the u8 is set then the following conversion to an u64 will
sign extend the value causing the upper 32 bits to be set in the result.
Fix this by casting the u8 value to a u64 before the shift. The commit
this fixes is pre-git history.
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/scsi/aic7xxx/aic79xx_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/aic7xxx/aic79xx_core.c b/drivers/scsi/aic7xxx/aic79xx_core.c
index 7e5044bf05c0..fa440c1b9baa 100644
--- a/drivers/scsi/aic7xxx/aic79xx_core.c
+++ b/drivers/scsi/aic7xxx/aic79xx_core.c
@@ -622,7 +622,7 @@ ahd_inq(struct ahd_softc *ahd, u_int port)
return ((ahd_inb(ahd, port))
| (ahd_inb(ahd, port+1) << 8)
| (ahd_inb(ahd, port+2) << 16)
- | (ahd_inb(ahd, port+3) << 24)
+ | (((uint64_t)ahd_inb(ahd, port+3)) << 24)
| (((uint64_t)ahd_inb(ahd, port+4)) << 32)
| (((uint64_t)ahd_inb(ahd, port+5)) << 40)
| (((uint64_t)ahd_inb(ahd, port+6)) << 48)
--
2.20.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH] scsi: aic7xxx: fix unintended sign extension on left shifts
@ 2019-10-14 12:56 ` Colin King
0 siblings, 0 replies; 10+ messages in thread
From: Colin King @ 2019-10-14 12:56 UTC (permalink / raw)
To: Hannes Reinecke, James E . J . Bottomley, Martin K . Petersen,
linux-scsi
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Shifting a u8 left will cause the value to be promoted to an integer. If
the top bit of the u8 is set then the following conversion to an u64 will
sign extend the value causing the upper 32 bits to be set in the result.
Fix this by casting the u8 value to a u64 before the shift. The commit
this fixes is pre-git history.
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/scsi/aic7xxx/aic79xx_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/aic7xxx/aic79xx_core.c b/drivers/scsi/aic7xxx/aic79xx_core.c
index 7e5044bf05c0..fa440c1b9baa 100644
--- a/drivers/scsi/aic7xxx/aic79xx_core.c
+++ b/drivers/scsi/aic7xxx/aic79xx_core.c
@@ -622,7 +622,7 @@ ahd_inq(struct ahd_softc *ahd, u_int port)
return ((ahd_inb(ahd, port))
| (ahd_inb(ahd, port+1) << 8)
| (ahd_inb(ahd, port+2) << 16)
- | (ahd_inb(ahd, port+3) << 24)
+ | (((uint64_t)ahd_inb(ahd, port+3)) << 24)
| (((uint64_t)ahd_inb(ahd, port+4)) << 32)
| (((uint64_t)ahd_inb(ahd, port+5)) << 40)
| (((uint64_t)ahd_inb(ahd, port+6)) << 48)
--
2.20.1
^ permalink raw reply related [flat|nested] 10+ messages in thread