LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] mptsas:  fix undefined behaviour of a shift of an int by more than 31 places
@ 2019-05-04 16:40 Colin King
  2019-05-05  3:34 ` James Bottomley
  0 siblings, 1 reply; 7+ messages in thread
From: Colin King @ 2019-05-04 16:40 UTC (permalink / raw)
  To: Sathya Prakash, Chaitra P B, Suganath Prabu Subramani,
	MPT-FusionLinux.pdl, linux-scsi
  Cc: kernel-janitors, linux-kernel

From: Colin Ian King <colin.king@canonical.com>

Currently the shift of int value 1 by more than 31 places can result
in undefined behaviour. Fix this by making the 1 a ULL value before the
shift operation.

Addresses-Coverity: ("Bad shift operation")
Fixes: 547f9a218436 ("[SCSI] mptsas: wide port support")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/message/fusion/mptsas.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index 6a79cd0ebe2b..51dcbbcf2518 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -854,7 +854,7 @@ mptsas_setup_wide_ports(MPT_ADAPTER *ioc, struct mptsas_portinfo *port_info)
 		    "%s: [%p]: deleting phy = %d\n",
 		    ioc->name, __func__, port_details, i));
 		port_details->num_phys--;
-		port_details->phy_bitmask &= ~ (1 << phy_info->phy_id);
+		port_details->phy_bitmask &= ~(1ULL << phy_info->phy_id);
 		memset(&phy_info->attached, 0, sizeof(struct mptsas_devinfo));
 		if (phy_info->phy) {
 			devtprintk(ioc, dev_printk(KERN_DEBUG,
@@ -889,7 +889,7 @@ mptsas_setup_wide_ports(MPT_ADAPTER *ioc, struct mptsas_portinfo *port_info)
 			port_details->port_info = port_info;
 			if (phy_info->phy_id < 64 )
 				port_details->phy_bitmask |=
-				    (1 << phy_info->phy_id);
+				    (1ULL << phy_info->phy_id);
 			phy_info->sas_port_add_phy=1;
 			dsaswideprintk(ioc, printk(MYIOC_s_DEBUG_FMT "\t\tForming port\n\t\t"
 			    "phy_id=%d sas_address=0x%018llX\n",
@@ -931,7 +931,7 @@ mptsas_setup_wide_ports(MPT_ADAPTER *ioc, struct mptsas_portinfo *port_info)
 			phy_info_cmp->port_details = port_details;
 			if (phy_info_cmp->phy_id < 64 )
 				port_details->phy_bitmask |=
-				(1 << phy_info_cmp->phy_id);
+				(1ULL << phy_info_cmp->phy_id);
 			port_details->num_phys++;
 		}
 	}
-- 
2.20.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] mptsas:  fix undefined behaviour of a shift of an int by more than 31 places
  2019-05-04 16:40 [PATCH] mptsas: fix undefined behaviour of a shift of an int by more than 31 places Colin King
@ 2019-05-05  3:34 ` James Bottomley
  2019-05-08 13:07   ` Colin Ian King
  0 siblings, 1 reply; 7+ messages in thread
From: James Bottomley @ 2019-05-05  3:34 UTC (permalink / raw)
  To: Colin King, Sathya Prakash, Chaitra P B,
	Suganath Prabu Subramani, MPT-FusionLinux.pdl, linux-scsi
  Cc: kernel-janitors, linux-kernel

On Sat, 2019-05-04 at 17:40 +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Currently the shift of int value 1 by more than 31 places can result
> in undefined behaviour. Fix this by making the 1 a ULL value before
> the shift operation.

Fusion SAS is pretty ancient.  I thought the largest one ever produced
had four phys, so how did you produce the overflow?

James


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] mptsas: fix undefined behaviour of a shift of an int by more than 31 places
  2019-05-05  3:34 ` James Bottomley
@ 2019-05-08 13:07   ` Colin Ian King
  2019-05-08 14:24     ` James Bottomley
  0 siblings, 1 reply; 7+ messages in thread
From: Colin Ian King @ 2019-05-08 13:07 UTC (permalink / raw)
  To: James Bottomley, Sathya Prakash, Chaitra P B,
	Suganath Prabu Subramani, MPT-FusionLinux.pdl, linux-scsi
  Cc: kernel-janitors, linux-kernel

On 05/05/2019 04:34, James Bottomley wrote:
> On Sat, 2019-05-04 at 17:40 +0100, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> Currently the shift of int value 1 by more than 31 places can result
>> in undefined behaviour. Fix this by making the 1 a ULL value before
>> the shift operation.
> 
> Fusion SAS is pretty ancient.  I thought the largest one ever produced
> had four phys, so how did you produce the overflow?

This was an issue found by static analysis with Coverity; so I guess
won't happen in the wild, in which case the patch could be ignored.

Colin

> 
> James
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] mptsas: fix undefined behaviour of a shift of an int by more than 31 places
  2019-05-08 13:07   ` Colin Ian King
@ 2019-05-08 14:24     ` James Bottomley
  2019-05-09 15:30       ` Hannes Reinecke
  0 siblings, 1 reply; 7+ messages in thread
From: James Bottomley @ 2019-05-08 14:24 UTC (permalink / raw)
  To: Colin Ian King, Sathya Prakash, Chaitra P B,
	Suganath Prabu Subramani, MPT-FusionLinux.pdl, linux-scsi
  Cc: kernel-janitors, linux-kernel

On Wed, 2019-05-08 at 14:07 +0100, Colin Ian King wrote:
> On 05/05/2019 04:34, James Bottomley wrote:
> > On Sat, 2019-05-04 at 17:40 +0100, Colin King wrote:
> > > From: Colin Ian King <colin.king@canonical.com>
> > > 
> > > Currently the shift of int value 1 by more than 31 places can
> > > result in undefined behaviour. Fix this by making the 1 a ULL
> > > value before the shift operation.
> > 
> > Fusion SAS is pretty ancient.  I thought the largest one ever
> > produced had four phys, so how did you produce the overflow?
> 
> This was an issue found by static analysis with Coverity; so I guess
> won't happen in the wild, in which case the patch could be ignored.

The point I was more making is that if we thought this could ever
happen in practice, we'd need more error handling than simply this:
we'd be setting the phy_bitmap to zero which would be every bit as bad
as some random illegal value.

James


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] mptsas: fix undefined behaviour of a shift of an int by more than 31 places
  2019-05-08 14:24     ` James Bottomley
@ 2019-05-09 15:30       ` Hannes Reinecke
  2019-05-09 15:42         ` James Bottomley
  0 siblings, 1 reply; 7+ messages in thread
From: Hannes Reinecke @ 2019-05-09 15:30 UTC (permalink / raw)
  To: James Bottomley, Colin Ian King, Sathya Prakash, Chaitra P B,
	Suganath Prabu Subramani, MPT-FusionLinux.pdl, linux-scsi
  Cc: kernel-janitors, linux-kernel

On 5/8/19 4:24 PM, James Bottomley wrote:
> On Wed, 2019-05-08 at 14:07 +0100, Colin Ian King wrote:
>> On 05/05/2019 04:34, James Bottomley wrote:
>>> On Sat, 2019-05-04 at 17:40 +0100, Colin King wrote:
>>>> From: Colin Ian King <colin.king@canonical.com>
>>>>
>>>> Currently the shift of int value 1 by more than 31 places can
>>>> result in undefined behaviour. Fix this by making the 1 a ULL
>>>> value before the shift operation.
>>>
>>> Fusion SAS is pretty ancient.  I thought the largest one ever
>>> produced had four phys, so how did you produce the overflow?
>>
>> This was an issue found by static analysis with Coverity; so I guess
>> won't happen in the wild, in which case the patch could be ignored.
> 
> The point I was more making is that if we thought this could ever
> happen in practice, we'd need more error handling than simply this:
> we'd be setting the phy_bitmap to zero which would be every bit as bad
> as some random illegal value.
> 
Thing is, mptsas is used as the default emulation in VMWare, and that 
does allow you to do some pretty weird configurations (I've found myself 
fixing a bug with SATA hotplug on mptsas once ...).
So I wouldn't discard this issue out of hand.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare@suse.de                              +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] mptsas: fix undefined behaviour of a shift of an int by more than 31 places
  2019-05-09 15:30       ` Hannes Reinecke
@ 2019-05-09 15:42         ` James Bottomley
  2019-05-09 15:58           ` Colin Ian King
  0 siblings, 1 reply; 7+ messages in thread
From: James Bottomley @ 2019-05-09 15:42 UTC (permalink / raw)
  To: Hannes Reinecke, Colin Ian King, Sathya Prakash, Chaitra P B,
	Suganath Prabu Subramani, MPT-FusionLinux.pdl, linux-scsi
  Cc: kernel-janitors, linux-kernel

On Thu, 2019-05-09 at 17:30 +0200, Hannes Reinecke wrote:
> On 5/8/19 4:24 PM, James Bottomley wrote:
> > On Wed, 2019-05-08 at 14:07 +0100, Colin Ian King wrote:
> > > On 05/05/2019 04:34, James Bottomley wrote:
> > > > On Sat, 2019-05-04 at 17:40 +0100, Colin King wrote:
> > > > > From: Colin Ian King <colin.king@canonical.com>
> > > > > 
> > > > > Currently the shift of int value 1 by more than 31 places can
> > > > > result in undefined behaviour. Fix this by making the 1 a ULL
> > > > > value before the shift operation.
> > > > 
> > > > Fusion SAS is pretty ancient.  I thought the largest one ever
> > > > produced had four phys, so how did you produce the overflow?
> > > 
> > > This was an issue found by static analysis with Coverity; so I
> > > guess won't happen in the wild, in which case the patch could be
> > > ignored.
> > 
> > The point I was more making is that if we thought this could ever
> > happen in practice, we'd need more error handling than simply this:
> > we'd be setting the phy_bitmap to zero which would be every bit as
> > bad as some random illegal value.
> > 
> 
> Thing is, mptsas is used as the default emulation in VMWare, and
> that does allow you to do some pretty weird configurations (I've
> found myself  fixing a bug with SATA hotplug on mptsas once ...).
> So I wouldn't discard this issue out of hand.

I'm not, I'm just saying the proposed fix is no fix at all since it
would just produce undefined behaviour in the driver.  I thought the
issue might have been coming from VMWare, which is why I asked how the
bug was seen.  The proper fix is probably to fail attachment if the phy
number goes over a fixed value (16 sounds reasonable) but if it's never
a problem in the field, I'm happy doing nothing because we have no real
idea what the reasonable value is.

James


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] mptsas: fix undefined behaviour of a shift of an int by more than 31 places
  2019-05-09 15:42         ` James Bottomley
@ 2019-05-09 15:58           ` Colin Ian King
  0 siblings, 0 replies; 7+ messages in thread
From: Colin Ian King @ 2019-05-09 15:58 UTC (permalink / raw)
  To: James Bottomley, Hannes Reinecke, Sathya Prakash, Chaitra P B,
	Suganath Prabu Subramani, MPT-FusionLinux.pdl, linux-scsi
  Cc: kernel-janitors, linux-kernel

On 09/05/2019 16:42, James Bottomley wrote:
> On Thu, 2019-05-09 at 17:30 +0200, Hannes Reinecke wrote:
>> On 5/8/19 4:24 PM, James Bottomley wrote:
>>> On Wed, 2019-05-08 at 14:07 +0100, Colin Ian King wrote:
>>>> On 05/05/2019 04:34, James Bottomley wrote:
>>>>> On Sat, 2019-05-04 at 17:40 +0100, Colin King wrote:
>>>>>> From: Colin Ian King <colin.king@canonical.com>
>>>>>>
>>>>>> Currently the shift of int value 1 by more than 31 places can
>>>>>> result in undefined behaviour. Fix this by making the 1 a ULL
>>>>>> value before the shift operation.
>>>>>
>>>>> Fusion SAS is pretty ancient.  I thought the largest one ever
>>>>> produced had four phys, so how did you produce the overflow?
>>>>
>>>> This was an issue found by static analysis with Coverity; so I
>>>> guess won't happen in the wild, in which case the patch could be
>>>> ignored.
>>>
>>> The point I was more making is that if we thought this could ever
>>> happen in practice, we'd need more error handling than simply this:
>>> we'd be setting the phy_bitmap to zero which would be every bit as
>>> bad as some random illegal value.
>>>
>>
>> Thing is, mptsas is used as the default emulation in VMWare, and
>> that does allow you to do some pretty weird configurations (I've
>> found myself  fixing a bug with SATA hotplug on mptsas once ...).
>> So I wouldn't discard this issue out of hand.
> 
> I'm not, I'm just saying the proposed fix is no fix at all since it
> would just produce undefined behaviour in the driver.  I thought the
> issue might have been coming from VMWare, which is why I asked how the
> bug was seen.  The proper fix is probably to fail attachment if the phy
> number goes over a fixed value (16 sounds reasonable) but if it's never
> a problem in the field, I'm happy doing nothing because we have no real
> idea what the reasonable value is.

I'm happy with my proposed fix being ignored and (if required) a more
appropriate fix developed by somebody who is more familiar with this
code than me.  Sometimes it is hard to determine if these static
analysis "issues" are fix worthy or not.

Colin

> 
> James
> 


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-05-09 15:58 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-04 16:40 [PATCH] mptsas: fix undefined behaviour of a shift of an int by more than 31 places Colin King
2019-05-05  3:34 ` James Bottomley
2019-05-08 13:07   ` Colin Ian King
2019-05-08 14:24     ` James Bottomley
2019-05-09 15:30       ` Hannes Reinecke
2019-05-09 15:42         ` James Bottomley
2019-05-09 15:58           ` Colin Ian King

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