[PATCH 1/1] ipr: Fix invalid array indexing for HRRQ

[PATCH 1/1] ipr: Fix invalid array indexing for HRRQ

[Brian King]

James,

Here is one more fix for a rather nasty bug where the ipr driver can start
accessing memory it doesn't own. I'd like to add to the queue
of ipr patches. There are now two patches on top of the previously submitted
series of four. If you want me to resend everything as a complete
series, please let me know.

Thanks,

Brian

8<

Fixes another signed / unsigned array indexing bug in the ipr driver.

Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
Tested-by: Wen Xiong <wenxiong@linux.vnet.ibm.com>
Cc: <stable@vger.kernel.org>
---

 drivers/scsi/ipr.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff -puN drivers/scsi/ipr.c~ipr_hrrq_index_fix drivers/scsi/ipr.c
--- linux/drivers/scsi/ipr.c~ipr_hrrq_index_fix	2015-06-23 11:43:18.151741523 -0500
+++ linux-bjking1/drivers/scsi/ipr.c	2015-06-23 11:43:18.157741435 -0500
@@ -1052,10 +1052,15 @@ static void ipr_send_blocking_cmd(struct
 
 static int ipr_get_hrrq_index(struct ipr_ioa_cfg *ioa_cfg)
 {
+	unsigned int hrrq;
+
 	if (ioa_cfg->hrrq_num == 1)
-		return 0;
-	else
-		return (atomic_add_return(1, &ioa_cfg->hrrq_index) % (ioa_cfg->hrrq_num - 1)) + 1;
+		hrrq = 0;
+	else {
+		hrrq = atomic_add_return(1, &ioa_cfg->hrrq_index);
+		hrrq = ((hrrq + 1) % (ioa_cfg->hrrq_num - 1)) + 1;
+	}
+	return hrrq;
 }
 
 /**
_

Re: [PATCH 1/1] ipr: Fix invalid array indexing for HRRQ

[Jiri Slaby]

On 06/25/2015, 03:44 PM, Brian King wrote:
> Fixes another signed / unsigned array indexing bug in the ipr driver.

Could you be more specific? Specifically, I fail to see why you do +1
twice now.

> --- linux/drivers/scsi/ipr.c~ipr_hrrq_index_fix	2015-06-23 11:43:18.151741523 -0500
> +++ linux-bjking1/drivers/scsi/ipr.c	2015-06-23 11:43:18.157741435 -0500
> @@ -1052,10 +1052,15 @@ static void ipr_send_blocking_cmd(struct
>  
>  static int ipr_get_hrrq_index(struct ipr_ioa_cfg *ioa_cfg)
>  {
> +	unsigned int hrrq;
> +
>  	if (ioa_cfg->hrrq_num == 1)
> -		return 0;
> -	else
> -		return (atomic_add_return(1, &ioa_cfg->hrrq_index) % (ioa_cfg->hrrq_num - 1)) + 1;
> +		hrrq = 0;
> +	else {
> +		hrrq = atomic_add_return(1, &ioa_cfg->hrrq_index);
> +		hrrq = ((hrrq + 1) % (ioa_cfg->hrrq_num - 1)) + 1;
> +	}
> +	return hrrq;

thanks,
-- 
js
suse labs

Re: [PATCH 1/1] ipr: Fix invalid array indexing for HRRQ

[Brian King]

On 06/25/2015 09:36 AM, Jiri Slaby wrote:
> On 06/25/2015, 03:44 PM, Brian King wrote:
>> Fixes another signed / unsigned array indexing bug in the ipr driver.
> 
> Could you be more specific? Specifically, I fail to see why you do +1
> twice now.

Sure. With the code that is currently upstream, when hrrq_index wraps, it
becomes a negative number. We do the modulo, but still have a negative number,
so we end up indexing backwards in the array. Given where the hrrq array is located
in memory, we probably won't actually reference memory we don't own, but nonetheless
ipr is still looking at data within struct ipr_ioa_cfg and interpreting it as
struct ipr_hrr_queue data, so bad things could certainly happen.

As far as the fix goes...

Each ipr adapter has anywhere from 1 to 16 HRRQs. By default, we use 2 on new adapters.
Let's take an example:

Assume ioa_cfg->hrrq_index=0x7fffffffe and ioa_cfg->hrrq_num=4:

The atomic_add_return will then return -1. We mod this with 3 and get -2, add one and
get -1 for an array index.

Some background on the different hrrq uses. On adapters which support more than a single
HRRQ, we dedicate HRRQ to adapter initialization and error interrupts so that we can
optimize the other queues for fast path I/O. So all normal I/O uses HRRQ 1-15. So we
want to spread the I/O requests across those HRRQs.

I should add here that with the default module parameter settings, this bug won't
hit, only when someone sets the ipr.number_of_msix parameter to a value larger than 3
is when bad things start to happen.

Thanks,

Brian


> 
>> --- linux/drivers/scsi/ipr.c~ipr_hrrq_index_fix	2015-06-23 11:43:18.151741523 -0500
>> +++ linux-bjking1/drivers/scsi/ipr.c	2015-06-23 11:43:18.157741435 -0500
>> @@ -1052,10 +1052,15 @@ static void ipr_send_blocking_cmd(struct
>>  
>>  static int ipr_get_hrrq_index(struct ipr_ioa_cfg *ioa_cfg)
>>  {
>> +	unsigned int hrrq;
>> +
>>  	if (ioa_cfg->hrrq_num == 1)
>> -		return 0;
>> -	else
>> -		return (atomic_add_return(1, &ioa_cfg->hrrq_index) % (ioa_cfg->hrrq_num - 1)) + 1;
>> +		hrrq = 0;
>> +	else {
>> +		hrrq = atomic_add_return(1, &ioa_cfg->hrrq_index);
>> +		hrrq = ((hrrq + 1) % (ioa_cfg->hrrq_num - 1)) + 1;
>> +	}
>> +	return hrrq;
> 
> thanks,
> 


-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

Re: [PATCH 1/1] ipr: Fix invalid array indexing for HRRQ

[Brian King]

On 06/25/2015 09:36 AM, Jiri Slaby wrote:
> On 06/25/2015, 03:44 PM, Brian King wrote:
>> Fixes another signed / unsigned array indexing bug in the ipr driver.
> 
> Could you be more specific? Specifically, I fail to see why you do +1
> twice now.

Regarding the extra +1, you are correct. Its not needed. Let me fix up the
commit comment and this and resend.

Thanks,

Brian


> 
>> --- linux/drivers/scsi/ipr.c~ipr_hrrq_index_fix	2015-06-23 11:43:18.151741523 -0500
>> +++ linux-bjking1/drivers/scsi/ipr.c	2015-06-23 11:43:18.157741435 -0500
>> @@ -1052,10 +1052,15 @@ static void ipr_send_blocking_cmd(struct
>>  
>>  static int ipr_get_hrrq_index(struct ipr_ioa_cfg *ioa_cfg)
>>  {
>> +	unsigned int hrrq;
>> +
>>  	if (ioa_cfg->hrrq_num == 1)
>> -		return 0;
>> -	else
>> -		return (atomic_add_return(1, &ioa_cfg->hrrq_index) % (ioa_cfg->hrrq_num - 1)) + 1;
>> +		hrrq = 0;
>> +	else {
>> +		hrrq = atomic_add_return(1, &ioa_cfg->hrrq_index);
>> +		hrrq = ((hrrq + 1) % (ioa_cfg->hrrq_num - 1)) + 1;
>> +	}
>> +	return hrrq;
> 
> thanks,
> 


-- 
Brian King
Power Linux I/O
IBM Linux Technology Center

[PATCHv2 1/1] ipr: Fix invalid array indexing for HRRQ

[Brian King]

Updated version per comments from Jiri Slaby. Thanks!

8<

Fixes another signed / unsigned array indexing bug in the ipr driver.
Currently, when hrrq_index wraps, it becomes a negative number. We
do the modulo, but still have a negative number, so we end up indexing
backwards in the array. Given where the hrrq array is located in memory,
we probably won't actually reference memory we don't own, but nonetheless
ipr is still looking at data within struct ipr_ioa_cfg and interpreting it as
struct ipr_hrr_queue data, so bad things could certainly happen.

Each ipr adapter has anywhere from 1 to 16 HRRQs. By default, we use 2 on new adapters.
Let's take an example:

Assume ioa_cfg->hrrq_index=0x7fffffffe and ioa_cfg->hrrq_num=4:

The atomic_add_return will then return -1. We mod this with 3 and get -2, add one and
get -1 for an array index.

On adapters which support more than a single HRRQ, we dedicate HRRQ to adapter
initialization and error interrupts so that we can optimize the other queues for
fast path I/O. So all normal I/O uses HRRQ 1-15. So we want to spread the I/O
requests across those HRRQs.

With the default module parameter settings, this bug won't hit, only when someone
sets the ipr.number_of_msix parameter to a value larger than 3 is when bad things
start to happen.

Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
Tested-by: Wen Xiong <wenxiong@linux.vnet.ibm.com>
Cc: <stable@vger.kernel.org>
---

 drivers/scsi/ipr.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff -puN drivers/scsi/ipr.c~ipr_hrrq_index_fix drivers/scsi/ipr.c
--- linux/drivers/scsi/ipr.c~ipr_hrrq_index_fix	2015-06-23 11:43:18.151741523 -0500
+++ linux-bjking1/drivers/scsi/ipr.c	2015-06-25 10:54:24.954615461 -0500
@@ -1052,10 +1052,15 @@ static void ipr_send_blocking_cmd(struct
 
 static int ipr_get_hrrq_index(struct ipr_ioa_cfg *ioa_cfg)
 {
+	unsigned int hrrq;
+
 	if (ioa_cfg->hrrq_num == 1)
-		return 0;
-	else
-		return (atomic_add_return(1, &ioa_cfg->hrrq_index) % (ioa_cfg->hrrq_num - 1)) + 1;
+		hrrq = 0;
+	else {
+		hrrq = atomic_add_return(1, &ioa_cfg->hrrq_index);
+		hrrq = (hrrq % (ioa_cfg->hrrq_num - 1)) + 1;
+	}
+	return hrrq;
 }
 
 /**
_

Re: [PATCHv2 1/1] ipr: Fix invalid array indexing for HRRQ

[wenxiong]

Reviewed-by: Wen Xiong <wenxiong@linux.vnet.ibm.com>

Thanks,
Wendy

Quoting Brian King <brking@linux.vnet.ibm.com>:

> Updated version per comments from Jiri Slaby. Thanks!
>
> 8<
>
> Fixes another signed / unsigned array indexing bug in the ipr driver.
> Currently, when hrrq_index wraps, it becomes a negative number. We
> do the modulo, but still have a negative number, so we end up indexing
> backwards in the array. Given where the hrrq array is located in memory,
> we probably won't actually reference memory we don't own, but nonetheless
> ipr is still looking at data within struct ipr_ioa_cfg and interpreting it as
> struct ipr_hrr_queue data, so bad things could certainly happen.
>
> Each ipr adapter has anywhere from 1 to 16 HRRQs. By default, we use  
> 2 on new adapters.
> Let's take an example:
>
> Assume ioa_cfg->hrrq_index=0x7fffffffe and ioa_cfg->hrrq_num=4:
>
> The atomic_add_return will then return -1. We mod this with 3 and  
> get -2, add one and
> get -1 for an array index.
>
> On adapters which support more than a single HRRQ, we dedicate HRRQ  
> to adapter
> initialization and error interrupts so that we can optimize the  
> other queues for
> fast path I/O. So all normal I/O uses HRRQ 1-15. So we want to spread the I/O
> requests across those HRRQs.
>
> With the default module parameter settings, this bug won't hit, only  
> when someone
> sets the ipr.number_of_msix parameter to a value larger than 3 is  
> when bad things
> start to happen.
>
> Signed-off-by: Brian King <brking@linux.vnet.ibm.com>
> Tested-by: Wen Xiong <wenxiong@linux.vnet.ibm.com>
> Cc: <stable@vger.kernel.org>
> ---
>
>  drivers/scsi/ipr.c |   11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff -puN drivers/scsi/ipr.c~ipr_hrrq_index_fix drivers/scsi/ipr.c
> --- linux/drivers/scsi/ipr.c~ipr_hrrq_index_fix	2015-06-23  
> 11:43:18.151741523 -0500
> +++ linux-bjking1/drivers/scsi/ipr.c	2015-06-25 10:54:24.954615461 -0500
> @@ -1052,10 +1052,15 @@ static void ipr_send_blocking_cmd(struct
>
>  static int ipr_get_hrrq_index(struct ipr_ioa_cfg *ioa_cfg)
>  {
> +	unsigned int hrrq;
> +
>  	if (ioa_cfg->hrrq_num == 1)
> -		return 0;
> -	else
> -		return (atomic_add_return(1, &ioa_cfg->hrrq_index) %  
> (ioa_cfg->hrrq_num - 1)) + 1;
> +		hrrq = 0;
> +	else {
> +		hrrq = atomic_add_return(1, &ioa_cfg->hrrq_index);
> +		hrrq = (hrrq % (ioa_cfg->hrrq_num - 1)) + 1;
> +	}
> +	return hrrq;
>  }
>
>  /**
> _

Re: [PATCHv2 1/1] ipr: Fix invalid array indexing for HRRQ

[Gabriel Krisman Bertazi]

Brian King <brking@linux.vnet.ibm.com> writes:

>  drivers/scsi/ipr.c |   11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)

Hi Brian,

The changes look good to me.  Please add

Reviewed-by: Gabriel Krisman Bertazi <krisman@linux.vnet.ibm.com>

Thanks,

-- 
Gabriel Krisman Bertazi