[PATCH] scsi: libsas: check lun number valid for ata device in sas_queuecommand()

[PATCH] scsi: libsas: check lun number valid for ata device in sas_queuecommand()

[Yufen Yu]

We found that offline a ata device on hisi sas control and then
scanning the host can probe 255 not exist devices into system.
It can be reproduced easily as following:

Some ata devices on hisi sas v3 control:
  [root@localhost ~]# lsscsi
  [2:0:0:0]    disk    ATA      Samsung SSD 860  2B6Q  /dev/sda
  [2:0:1:0]    disk    ATA      WDC WD2003FYYS-3 1D01  /dev/sdb
  [2:0:2:0]    disk    SEAGATE  ST600MM0006      B001  /dev/sdc

 1) echo "offline" > /sys/block/sdb/device/state
 2) echo "- - -" > /sys/class/scsi_host/host2/scan

Then, we can see another 255 not exist device in system:
  [root@localhost ~]# lsscsi
  [2:0:0:0]    disk    ATA      Samsung SSD 860  2B6Q  /dev/sda
  [2:0:1:0]    disk    ATA      WDC WD2003FYYS-3 1D01  /dev/sdb
  [2:0:1:1]    disk    ATA      WDC WD2003FYYS-3 1D01  /dev/sdh
  ...
  [2:0:1:254]  disk    ATA      WDC WD2003FYYS-3 1D01  /dev/sdja
  [2:0:1:255]  disk    ATA      WDC WD2003FYYS-3 1D01  /dev/sdjb

When we try to scan the host, REPORT LUN command issued to the
offline device (sdb) will return fail. Then it tries to do a
sequential scan. Since only one ata device, any INQUIRY command
will be issued to the only device (i.e. lun 0) and return success,
no matter the lun number. So, the device whose lun number is not
zero will also be probed into system.

We fix the probe by checking lun number valid in sas_queuecommand.
Any lun number is not equal '0' will return fail.

Signed-off-by: Wu Bo <wubo40@huawei.com>
Signed-off-by: Yufen Yu <yuyufen@huawei.com>
---
 drivers/scsi/libsas/sas_scsi_host.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index 1bf939818c98..62a01d11df96 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -174,6 +174,12 @@ int sas_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 	}
 
 	if (dev_is_sata(dev)) {
+		/* sas ata just have one lun */
+		if (cmd->device->lun != 0) {
+			cmd->result = (DID_BAD_TARGET << 16);
+			cmd->scsi_done(cmd);
+			return res;
+		}
 		spin_lock_irq(dev->sata_dev.ap->lock);
 		res = ata_sas_queuecmd(cmd, dev->sata_dev.ap);
 		spin_unlock_irq(dev->sata_dev.ap->lock);
-- 
2.25.4

Re: [PATCH] scsi: libsas: check lun number valid for ata device in sas_queuecommand()

[John Garry]

On 01/06/2021 08:04, Yufen Yu wrote:
> We found that offline a ata device on hisi sas control and then
> scanning the host can probe 255 not exist devices into system.
> It can be reproduced easily as following:
> 
> Some ata devices on hisi sas v3 control:
>    [root@localhost ~]# lsscsi
>    [2:0:0:0]    disk    ATA      Samsung SSD 860  2B6Q  /dev/sda
>    [2:0:1:0]    disk    ATA      WDC WD2003FYYS-3 1D01  /dev/sdb
>    [2:0:2:0]    disk    SEAGATE  ST600MM0006      B001  /dev/sdc
> 
>   1) echo "offline" > /sys/block/sdb/device/state
>   2) echo "- - -" > /sys/class/scsi_host/host2/scan
> 
> Then, we can see another 255 not exist device in system:
>    [root@localhost ~]# lsscsi
>    [2:0:0:0]    disk    ATA      Samsung SSD 860  2B6Q  /dev/sda
>    [2:0:1:0]    disk    ATA      WDC WD2003FYYS-3 1D01  /dev/sdb
>    [2:0:1:1]    disk    ATA      WDC WD2003FYYS-3 1D01  /dev/sdh
>    ...
>    [2:0:1:254]  disk    ATA      WDC WD2003FYYS-3 1D01  /dev/sdja
>    [2:0:1:255]  disk    ATA      WDC WD2003FYYS-3 1D01  /dev/sdjb
> 
> When we try to scan the host, REPORT LUN command issued to the
> offline device (sdb) will return fail. Then it tries to do a
> sequential scan. Since only one ata device, any INQUIRY command
> will be issued to the only device (i.e. lun 0) and return success,
> no matter the lun number. So, the device whose lun number is not
> zero will also be probed into system.
> 
> We fix the probe by checking lun number valid in sas_queuecommand.
> Any lun number is not equal '0' will return fail.
> 
> Signed-off-by: Wu Bo <wubo40@huawei.com>
> Signed-off-by: Yufen Yu <yuyufen@huawei.com>

As mentioned privately, we could alternatively add a check in a slave 
alloc callback, like:

int sas_slave_alloc(struct scsi_device *sdev)
{
	if (dev_is_sata(sdev_to_domain_dev(sdev)) && sdev->lun)
		return -ENXIO;

	return 0;
}

This avoids touching the queuecommand fastpath. But not sure it is worth it.

BTW, please mention that we are doing similar to commit 2fc62e2ac, to 
cut down the commit log a bit.

> ---
>   drivers/scsi/libsas/sas_scsi_host.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
> index 1bf939818c98..62a01d11df96 100644
> --- a/drivers/scsi/libsas/sas_scsi_host.c
> +++ b/drivers/scsi/libsas/sas_scsi_host.c
> @@ -174,6 +174,12 @@ int sas_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>   	}
>   
>   	if (dev_is_sata(dev)) {
> +		/* sas ata just have one lun */
> +		if (cmd->device->lun != 0) {
> +			cmd->result = (DID_BAD_TARGET << 16);
> +			cmd->scsi_done(cmd);
> +			return res;

goto out_done is better

> +		}
>   		spin_lock_irq(dev->sata_dev.ap->lock);
>   		res = ata_sas_queuecmd(cmd, dev->sata_dev.ap);
>   		spin_unlock_irq(dev->sata_dev.ap->lock);
>