Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1601819 > unrolled thread

Re: [PATCH] scsi: sr: fix oob access in get_capabilities

Started by"Martin K. Petersen" <martin.petersen@oracle.com>
First post2017-03-16 01:10 +0100
Last post2017-03-21 03:40 +0100
Articles 6 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] scsi: sr: fix oob access in get_capabilities "Martin K. Petersen" <martin.petersen@oracle.com> - 2017-03-16 01:10 +0100
    Re: [PATCH] scsi: sr: fix oob access in get_capabilities Kefeng Wang <wangkefeng.wang@huawei.com> - 2017-03-16 06:30 +0100
      Re: [PATCH] scsi: sr: fix oob access in get_capabilities "Martin K. Petersen" <martin.petersen@oracle.com> - 2017-03-18 02:00 +0100
        Re: [PATCH] scsi: sr: fix oob access in get_capabilities Kefeng Wang <wangkefeng.wang@huawei.com> - 2017-03-20 07:10 +0100
          Re: [PATCH] scsi: sr: fix oob access in get_capabilities "Martin K. Petersen" <martin.petersen@oracle.com> - 2017-03-20 15:30 +0100
            Re: [PATCH] scsi: sr: fix oob access in get_capabilities Kefeng Wang <wangkefeng.wang@huawei.com> - 2017-03-21 03:40 +0100

#1601819 — Re: [PATCH] scsi: sr: fix oob access in get_capabilities

From"Martin K. Petersen" <martin.petersen@oracle.com>
Date2017-03-16 01:10 +0100
SubjectRe: [PATCH] scsi: sr: fix oob access in get_capabilities
Message-ID<tlr9o-6R3-9@gated-at.bofh.it>
Kefeng Wang <wangkefeng.wang@huawei.com> writes:

Kefeng,

> 'n = header_length + block_descriptor_length' could be greater than 512,
> and will lead to oob access, so enlarge transfer buffer to fix it.

Can you share the output of sg_modes -p 0x2a /dev/srN for the offending
drive?

This mode page is usually much smaller than 512 bytes (typically between
32 and 128 bytes).

-- 
Martin K. Petersen	Oracle Linux Engineering

[toc] | [next] | [standalone]

#1601931

FromKefeng Wang <wangkefeng.wang@huawei.com>
Date2017-03-16 06:30 +0100
Message-ID<tlw93-1WG-13@gated-at.bofh.it>
In reply to#1601819

On 2017/3/16 8:07, Martin K. Petersen wrote:
> Kefeng Wang <wangkefeng.wang@huawei.com> writes:
> 
> Kefeng,
> 
>> 'n = header_length + block_descriptor_length' could be greater than 512,
>> and will lead to oob access, so enlarge transfer buffer to fix it.
> 
> Can you share the output of sg_modes -p 0x2a /dev/srN for the offending
> drive?


root@localhost ~]# sg_modes -p 0x2a /dev/sr0
    QEMU      QEMU DVD-ROM      0.15   peripheral_type: cd/dvd [0x5]
Mode parameter header from MODE SENSE(10):
Invalid block descriptor length=512, ignore
  Mode data length=36, medium type=0x70, specific param=0x00, longlba=0
  Block descriptor length=0
>> MM capabilities and mechanical status (obsolete), page_control: current
 00     2a 12 00 00 71 60 29 00  02 c2 00 02 02 00 02 c2
 10     00 00 00 00
Unexpectedly received extra mode page responses, ignore

note: the issue was found in guestos?  maybe some bugs exist in qemu?

Kefeng

> 
> This mode page is usually much smaller than 512 bytes (typically between
> 32 and 128 bytes).
>

[toc] | [prev] | [next] | [standalone]

#1603684

From"Martin K. Petersen" <martin.petersen@oracle.com>
Date2017-03-18 02:00 +0100
Message-ID<tmaSR-6Db-15@gated-at.bofh.it>
In reply to#1601931
Kefeng Wang <wangkefeng.wang@huawei.com> writes:

Kefeng,

> root@localhost ~]# sg_modes -p 0x2a /dev/sr0
>     QEMU      QEMU DVD-ROM      0.15   peripheral_type: cd/dvd [0x5]
> Mode parameter header from MODE SENSE(10):
> Invalid block descriptor length=512, ignore
>   Mode data length=36, medium type=0x70, specific param=0x00, longlba=0
>   Block descriptor length=0
>>> MM capabilities and mechanical status (obsolete), page_control: current
>  00     2a 12 00 00 71 60 29 00  02 c2 00 02 02 00 02 c2
>  10     00 00 00 00
> Unexpectedly received extra mode page responses, ignore

That looks pretty broken.

Could you try the following patch?

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f41e6b84a1bd..51a4ce094450 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2485,6 +2485,10 @@ scsi_mode_sense(struct scsi_device *sdev, int dbd, int modepage,
 		goto retry;
 	}
 
+	if (data->length > len ||
+	    data->header_length + data->block_descriptor_length > data->length)
+		return 0;
+
 	return result;
 }
 EXPORT_SYMBOL(scsi_mode_sense);

[toc] | [prev] | [next] | [standalone]

#1604190

FromKefeng Wang <wangkefeng.wang@huawei.com>
Date2017-03-20 07:10 +0100
Message-ID<tmYFY-yg-11@gated-at.bofh.it>
In reply to#1603684

On 2017/3/18 7:29, Martin K. Petersen wrote:
> Kefeng Wang <wangkefeng.wang@huawei.com> writes:
> 
> Kefeng,
> 
>> root@localhost ~]# sg_modes -p 0x2a /dev/sr0
>>     QEMU      QEMU DVD-ROM      0.15   peripheral_type: cd/dvd [0x5]
>> Mode parameter header from MODE SENSE(10):
>> Invalid block descriptor length=512, ignore
>>   Mode data length=36, medium type=0x70, specific param=0x00, longlba=0
>>   Block descriptor length=0
>>>> MM capabilities and mechanical status (obsolete), page_control: current
>>  00     2a 12 00 00 71 60 29 00  02 c2 00 02 02 00 02 c2
>>  10     00 00 00 00
>> Unexpectedly received extra mode page responses, ignore
> 
> That looks pretty broken.
> 
> Could you try the following patch?

The issue still exists, the patch return zero in scsi_mode_sense(), but zero means
SAM_STAT_GOOD in scsi_status_is_good(), so n will be still bigger than 512;

Thanks,

[toc] | [prev] | [next] | [standalone]

#1604622

From"Martin K. Petersen" <martin.petersen@oracle.com>
Date2017-03-20 15:30 +0100
Message-ID<tn6tP-5Wi-5@gated-at.bofh.it>
In reply to#1604190
Kefeng Wang <wangkefeng.wang@huawei.com> writes:

Kefeng,

> The issue still exists, the patch return zero in scsi_mode_sense(), but zero means
> SAM_STAT_GOOD in scsi_status_is_good(), so n will be still bigger than 512;

OK, I checked the other users of scsi_mode_sense(). So let's keep this
fix local to sr.c for now.

How about the following?


scsi: sr: Sanity check returned mode data
    
Kefeng Wang discovered that old versions of the QEMU CD driver would
return mangled mode data causing us to walk off the end of the buffer in
an attempt to parse it. Sanity check the returned mode sense data.
    
Cc: <stable@vger.kernel.org>
Reported-by: Kefeng Wang <wangkefeng.wang@huawei.com>
Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 0b29b9329b1c..a8f630213a1a 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -836,6 +836,7 @@ static void get_capabilities(struct scsi_cd *cd)
 	unsigned char *buffer;
 	struct scsi_mode_data data;
 	struct scsi_sense_hdr sshdr;
+	unsigned int ms_len = 128;
 	int rc, n;
 
 	static const char *loadmech[] =
@@ -862,10 +863,11 @@ static void get_capabilities(struct scsi_cd *cd)
 	scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
 
 	/* ask for mode page 0x2a */
-	rc = scsi_mode_sense(cd->device, 0, 0x2a, buffer, 128,
+	rc = scsi_mode_sense(cd->device, 0, 0x2a, buffer, ms_len,
 			     SR_TIMEOUT, 3, &data, NULL);
 
-	if (!scsi_status_is_good(rc)) {
+	if (!scsi_status_is_good(rc) || data.length > ms_len ||
+	    data.header_length + data.block_descriptor_length > data.length) {
 		/* failed, drive doesn't have capabilities mode page */
 		cd->cdi.speed = 1;
 		cd->cdi.mask |= (CDC_CD_R | CDC_CD_RW | CDC_DVD_R |

[toc] | [prev] | [next] | [standalone]

#1605265

FromKefeng Wang <wangkefeng.wang@huawei.com>
Date2017-03-21 03:40 +0100
Message-ID<tnhSh-5lt-7@gated-at.bofh.it>
In reply to#1604622

On 2017/3/20 22:29, Martin K. Petersen wrote:
> Kefeng Wang <wangkefeng.wang@huawei.com> writes:
> 
> Kefeng,
> 
>> The issue still exists, the patch return zero in scsi_mode_sense(), but zero means
>> SAM_STAT_GOOD in scsi_status_is_good(), so n will be still bigger than 512;
> 
> OK, I checked the other users of scsi_mode_sense(). So let's keep this
> fix local to sr.c for now.
> 
> How about the following?
> 
> 
> scsi: sr: Sanity check returned mode data
>     
> Kefeng Wang discovered that old versions of the QEMU CD driver would
> return mangled mode data causing us to walk off the end of the buffer in
> an attempt to parse it. Sanity check the returned mode sense data.
>     
> Cc: <stable@vger.kernel.org>
> Reported-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> 
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 0b29b9329b1c..a8f630213a1a 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -836,6 +836,7 @@ static void get_capabilities(struct scsi_cd *cd)
>  	unsigned char *buffer;
>  	struct scsi_mode_data data;
>  	struct scsi_sense_hdr sshdr;
> +	unsigned int ms_len = 128;
>  	int rc, n;
>  
>  	static const char *loadmech[] =
> @@ -862,10 +863,11 @@ static void get_capabilities(struct scsi_cd *cd)
>  	scsi_test_unit_ready(cd->device, SR_TIMEOUT, MAX_RETRIES, &sshdr);
>  
>  	/* ask for mode page 0x2a */
> -	rc = scsi_mode_sense(cd->device, 0, 0x2a, buffer, 128,
> +	rc = scsi_mode_sense(cd->device, 0, 0x2a, buffer, ms_len,
>  			     SR_TIMEOUT, 3, &data, NULL);
>  

move n = data.header_length + data.block_descriptor_length; here,

> -	if (!scsi_status_is_good(rc)) {
> +	if (!scsi_status_is_good(rc) || data.length > ms_len ||
> +	    data.header_length + data.block_descriptor_length > data.length) {

n > data.length

Tested-by: Kefeng Wang <wangkefeng.wang@huawei.com>

Thanks,
Kefeng

>  		/* failed, drive doesn't have capabilities mode page */
>  		cd->cdi.speed = 1;
>  		cd->cdi.mask |= (CDC_CD_R | CDC_CD_RW | CDC_DVD_R |
> 
> 
> 
> .
>

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web