| Message ID | 20181025151334.15622-1-colin.king@canonical.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: aic7xxx: Fix unintended sign extension issue | expand |
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
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
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)
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)