Re: [PATCH] ata: libata-scsi: avoid READ CAPACITY 16 underflow

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ata: libata-scsi: ata_scsiop_read_cap() reports 1 block when n_sectors is 0
--

commit 2baef3c64e10a35b658037f12f8a9575c158e978
Author: shao.mingyin@zte.com.cn <shao.mingyin@zte.com.cn>

ata: libata-scsi: avoid READ CAPACITY 16 underflow

This commit updates ata_scsiop_read_cap() to set last_lba to 0 instead of
underflowing dev->n_sectors - 1 when dev->n_sectors is zero, such as on
security-locked drives.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index a61fb03312793..eaf86dc2cefc0 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -2984,7 +2984,7 @@ static unsigned int ata_scsiop_read_cap(struct ata_device *dev,
>  					struct scsi_cmnd *cmd, u8 *rbuf)
>  {
>  	u8 *scsicmd = cmd->cmnd;
> -	u64 last_lba = dev->n_sectors - 1; /* LBA of the last block */
> +	u64 last_lba = dev->n_sectors ? dev->n_sectors - 1 : 0; /* LBA of the last block */

[Severity: High]
Does setting last_lba to 0 when dev->n_sectors is 0 cause the SCSI disk
driver to report a capacity of 1 block instead of 0 blocks?

In the SCSI READ CAPACITY (10) and READ CAPACITY (16) parameter data, the
returned logical block address is the 0-based LBA of the last logical block
on the medium (capacity - 1), so returning 0 indicates that LBA 0 is a valid
block on a 1-block disk.

When sd_revalidate_disk() -> sd_read_capacity() calls read_capacity_16() or
read_capacity_10() in drivers/scsi/sd.c, both functions add 1 to the
returned LBA:

drivers/scsi/sd.c:read_capacity_16() {
	...
	lba = get_unaligned_be64(&buffer[0]);
	...
	sdkp->capacity = lba + 1;
}

Previously, when dev->n_sectors was 0, last_lba underflowed to
0xffffffffffffffffULL, and lba + 1 in read_capacity_16() wrapped around in
64-bit unsigned arithmetic to set sdkp->capacity = 0.

With last_lba set to 0, sdkp->capacity becomes 1, and sd_revalidate_disk()
sets a non-zero gendisk capacity via set_capacity_and_notify(). In
drivers/ata/libata-core.c, ata_dev_configure() sets dev->n_sectors = 0 on
security-locked drives specifically to prevent partition scanning:

drivers/ata/libata-core.c:ata_dev_configure() {
	...
		if (ata_id_is_locked(id)) {
			/*
			 * If Security locked, set capacity to zero to prevent
			 * any I/O, e.g. partition scanning, as any I/O to a
			 * locked drive will result in user visible errors.
			 */
			ata_dev_info(dev,
				"Security locked, setting capacity to zero\n");
			dev->n_sectors = 0;
		}
}

Because bdev_disk_changed() in block/partitions/core.c checks
if (get_capacity(disk)) before calling blk_add_partitions(disk), won't a
reported capacity of 1 block cause the block layer to issue partition-scan
reads to LBA 0 on security-locked drives, resulting in I/O errors?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103185346937YqEZr3t5lq7FdnTzM3-TY@zte.com.cn?part=1