[PATCH] mptsas: fix undefined behaviour of a shift of an int by more than 31 places

[PATCH] mptsas: fix undefined behaviour of a shift of an int by more than 31 places

[Colin King]

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

Re: [PATCH] mptsas: fix undefined behaviour of a shift of an int by more than 31 places

[James Bottomley]

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

Re: [PATCH] mptsas: fix undefined behaviour of a shift of an int by more than 31 places

[Colin Ian King]

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
> 

Re: [PATCH] mptsas: fix undefined behaviour of a shift of an int by more than 31 places

[James Bottomley]

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

Re: [PATCH] mptsas: fix undefined behaviour of a shift of an int by more than 31 places

[Hannes Reinecke]

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)

Re: [PATCH] mptsas: fix undefined behaviour of a shift of an int by more than 31 places

[James Bottomley]

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

Re: [PATCH] mptsas: fix undefined behaviour of a shift of an int by more than 31 places

[Colin Ian King]

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
>