diff mbox series

scsi: target: pscsi: Fix possible null-pointer dereference in pscsi_complete_cmd()

Message ID 20210807134651.245436-1-islituo@gmail.com
State New
Headers show
Series scsi: target: pscsi: Fix possible null-pointer dereference in pscsi_complete_cmd() | expand

Commit Message

Tuo Li Aug. 7, 2021, 1:46 p.m. UTC
The return value of transport_kmap_data_sg() is assigned to the variable
buf:
  buf = transport_kmap_data_sg(cmd);

And then it is checked:
  if (!buf) {

This indicates that buf can be NULL. However, it is dereferenced in the
following statements:
  if (!(buf[3] & 0x80))
    buf[3] |= 0x80;
  if (!(buf[2] & 0x80))
	buf[2] |= 0x80;

To fix these possible null-pointer dereferences, dereference buf only when
it is not NULL.

Reported-by: TOTE Robot <oslab@tsinghua.edu.cn>
Signed-off-by: Tuo Li <islituo@gmail.com>
---
 drivers/target/target_core_pscsi.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

Bodo Stroesser Aug. 9, 2021, 10:36 a.m. UTC | #1
On 07.08.21 15:46, Tuo Li wrote:
> The return value of transport_kmap_data_sg() is assigned to the variable

> buf:

>    buf = transport_kmap_data_sg(cmd);

> 

> And then it is checked:

>    if (!buf) {

> 

> This indicates that buf can be NULL. However, it is dereferenced in the

> following statements:

>    if (!(buf[3] & 0x80))

>      buf[3] |= 0x80;

>    if (!(buf[2] & 0x80))

> 	buf[2] |= 0x80;

> 

> To fix these possible null-pointer dereferences, dereference buf only when

> it is not NULL.

> 

> Reported-by: TOTE Robot <oslab@tsinghua.edu.cn>

> Signed-off-by: Tuo Li <islituo@gmail.com>

> ---

>   drivers/target/target_core_pscsi.c | 14 +++++++-------

>   1 file changed, 7 insertions(+), 7 deletions(-)

> 

> diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c

> index 2629d2ef3970..560815729182 100644

> --- a/drivers/target/target_core_pscsi.c

> +++ b/drivers/target/target_core_pscsi.c

> @@ -620,14 +620,14 @@ static void pscsi_complete_cmd(struct se_cmd *cmd, u8 scsi_status,

>   			buf = transport_kmap_data_sg(cmd);

>   			if (!buf) {

>   				; /* XXX: TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE */

> -			}

> -

> -			if (cdb[0] == MODE_SENSE_10) {

> -				if (!(buf[3] & 0x80))

> -					buf[3] |= 0x80;

>   			} else {

> -				if (!(buf[2] & 0x80))

> -					buf[2] |= 0x80;

> +				if (cdb[0] == MODE_SENSE_10) {

> +					if (!(buf[3] & 0x80))

> +						buf[3] |= 0x80;

> +				} else {

> +					if (!(buf[2] & 0x80))

> +						buf[2] |= 0x80;

> +				}

>   			}

>   

>   			transport_kunmap_data_sg(cmd);

> 


I'm wondering whether we should better put the
transport_kunmap_data_sg into the else-branch of the if (!buf)?
AFAICS, calling it after transport_kmap_data_sg failed does not
cause problems, but I feel it would be cleaner.

Otherwise it looks good to me.
Tuo Li Aug. 9, 2021, 11:50 a.m. UTC | #2
Thanks for your feedback. We will prepare a V2 patch and put the 
transport_kunmap_data_sg()
into the else-branch of the if (!buf).

Best wishes,
Tuo Li

On 2021/8/9 18:36, Bodo Stroesser wrote:
> On 07.08.21 15:46, Tuo Li wrote:

>> The return value of transport_kmap_data_sg() is assigned to the variable

>> buf:

>>    buf = transport_kmap_data_sg(cmd);

>>

>> And then it is checked:

>>    if (!buf) {

>>

>> This indicates that buf can be NULL. However, it is dereferenced in the

>> following statements:

>>    if (!(buf[3] & 0x80))

>>      buf[3] |= 0x80;

>>    if (!(buf[2] & 0x80))

>>     buf[2] |= 0x80;

>>

>> To fix these possible null-pointer dereferences, dereference buf only 

>> when

>> it is not NULL.

>>

>> Reported-by: TOTE Robot <oslab@tsinghua.edu.cn>

>> Signed-off-by: Tuo Li <islituo@gmail.com>

>> ---

>>   drivers/target/target_core_pscsi.c | 14 +++++++-------

>>   1 file changed, 7 insertions(+), 7 deletions(-)

>>

>> diff --git a/drivers/target/target_core_pscsi.c 

>> b/drivers/target/target_core_pscsi.c

>> index 2629d2ef3970..560815729182 100644

>> --- a/drivers/target/target_core_pscsi.c

>> +++ b/drivers/target/target_core_pscsi.c

>> @@ -620,14 +620,14 @@ static void pscsi_complete_cmd(struct se_cmd 

>> *cmd, u8 scsi_status,

>>               buf = transport_kmap_data_sg(cmd);

>>               if (!buf) {

>>                   ; /* XXX: TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE */

>> -            }

>> -

>> -            if (cdb[0] == MODE_SENSE_10) {

>> -                if (!(buf[3] & 0x80))

>> -                    buf[3] |= 0x80;

>>               } else {

>> -                if (!(buf[2] & 0x80))

>> -                    buf[2] |= 0x80;

>> +                if (cdb[0] == MODE_SENSE_10) {

>> +                    if (!(buf[3] & 0x80))

>> +                        buf[3] |= 0x80;

>> +                } else {

>> +                    if (!(buf[2] & 0x80))

>> +                        buf[2] |= 0x80;

>> +                }

>>               }

>>                 transport_kunmap_data_sg(cmd);

>>

>

> I'm wondering whether we should better put the

> transport_kunmap_data_sg into the else-branch of the if (!buf)?

> AFAICS, calling it after transport_kmap_data_sg failed does not

> cause problems, but I feel it would be cleaner.

>

> Otherwise it looks good to me.
diff mbox series

Patch

diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
index 2629d2ef3970..560815729182 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -620,14 +620,14 @@  static void pscsi_complete_cmd(struct se_cmd *cmd, u8 scsi_status,
 			buf = transport_kmap_data_sg(cmd);
 			if (!buf) {
 				; /* XXX: TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE */
-			}
-
-			if (cdb[0] == MODE_SENSE_10) {
-				if (!(buf[3] & 0x80))
-					buf[3] |= 0x80;
 			} else {
-				if (!(buf[2] & 0x80))
-					buf[2] |= 0x80;
+				if (cdb[0] == MODE_SENSE_10) {
+					if (!(buf[3] & 0x80))
+						buf[3] |= 0x80;
+				} else {
+					if (!(buf[2] & 0x80))
+						buf[2] |= 0x80;
+				}
 			}
 
 			transport_kunmap_data_sg(cmd);