linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] scsi: aic7xxx: Fix unintended sign extension issue
@ 2018-10-25 15:13 Colin King
  2018-10-25 15:32 ` Joe Perches
  2018-10-25 15:32 ` James Bottomley
  0 siblings, 2 replies; 4+ 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	[flat|nested] 4+ messages in thread

* Re: [PATCH] scsi: aic7xxx: Fix unintended sign extension issue
  2018-10-25 15:13 [PATCH] scsi: aic7xxx: Fix unintended sign extension issue Colin King
@ 2018-10-25 15:32 ` Joe Perches
  2018-10-25 15:32 ` James Bottomley
  1 sibling, 0 replies; 4+ 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] 4+ messages in thread

* Re: [PATCH] scsi: aic7xxx: Fix unintended sign extension issue
  2018-10-25 15:13 [PATCH] scsi: aic7xxx: Fix unintended sign extension issue Colin King
  2018-10-25 15:32 ` Joe Perches
@ 2018-10-25 15:32 ` James Bottomley
  2018-10-25 15:54   ` David Laight
  1 sibling, 1 reply; 4+ 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] 4+ 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
  0 siblings, 0 replies; 4+ 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
  Cc: kernel-janitors, linux-kernel

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] 4+ messages in thread

end of thread, other threads:[~2018-10-25 15:54 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-25 15:13 [PATCH] scsi: aic7xxx: Fix unintended sign extension issue Colin King
2018-10-25 15:32 ` Joe Perches
2018-10-25 15:32 ` James Bottomley
2018-10-25 15:54   ` David Laight

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