From: Colin Ian King <[email protected]>
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 <[email protected]>
---
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
On Thu, 2018-10-25 at 16:13 +0100, Colin King wrote:
> From: Colin Ian King <[email protected]>
>
> 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
On Thu, 2018-10-25 at 16:13 +0100, Colin King wrote:
> From: Colin Ian King <[email protected]>
>
> 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
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 <[email protected]>
> >
> > 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)