[PATCH v2] ata: libata-scsi: limit simulated SCSI command copy to response length

[PATCH v2] ata: libata-scsi: limit simulated SCSI command copy to response length

[Karuna Ramkumar]

The function ata_scsi_rbuf_fill() is used to copy the response of
emulated SCSI commands from ata_scsi_rbuf to the SCSI command's
scatterlist.

Currently, sg_copy_from_buffer() is called with the size argument
set to ATA_SCSI_RBUF_SIZE (2048 bytes). Since ata_scsi_rbuf is
zeroed out before the simulation actor is invoked, copying the
full buffer size causes the remainder of the SCSI command's
transfer buffer (beyond the actual response length 'len') to be
overwritten with zeroes. This clobbers any pre-existing sentinel
values or data in the caller's buffer tail, even though the
correct residual count is reported via scsi_set_resid().

Fix this by passing the actual response length 'len' as the copy
size to sg_copy_from_buffer(), ensuring that the tail of the
caller's buffer remains untouched. Also, add a defensive check
to ensure that the actor does not return a length exceeding the
static buffer capacity. If this occurs, trigger a WARN_ON(),
fail the command with an aborted command error, and return
immediately without copying any data.

The fix was tested by invoking an SCSI SG_IO INQUIRY on
an ATA disk on vanilla build, and build with the fix. Confirmed
that the input buffer's tail end remains unmodified with the fix.

Fixes: 5251ae224d8d ("ata: libata-scsi: Return residual for emulated SCSI commands")
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Karuna Ramkumar <rkaruna@google.com>
---
Changes in v2:
   - Instead of capping 'len' and continuing when 'len > ATA_SCSI_RBUF_SIZE',
     fail the SCSI command with an ABORTED_COMMAND error and return immediately,
     as suggested by Damien Le Moal.
   - Link to v1: https://lore.kernel.org/all/20260629175024.2187500-1-rkaruna@google.com/

 drivers/ata/libata-scsi.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 72744a07c140..052a71540137 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2018,8 +2018,13 @@ static void ata_scsi_rbuf_fill(struct ata_device *dev, struct scsi_cmnd *cmd,
 	memset(ata_scsi_rbuf, 0, ATA_SCSI_RBUF_SIZE);
 	len = actor(dev, cmd, ata_scsi_rbuf);
 	if (len) {
+		if (WARN_ON(len > ATA_SCSI_RBUF_SIZE)) {
+			ata_scsi_set_sense(dev, cmd, ABORTED_COMMAND, 0, 0);
+			spin_unlock_irqrestore(&ata_scsi_rbuf_lock, flags);
+			return;
+		}
 		sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd),
-				    ata_scsi_rbuf, ATA_SCSI_RBUF_SIZE);
+				    ata_scsi_rbuf, len);
 		cmd->result = SAM_STAT_GOOD;
 		if (scsi_bufflen(cmd) > len)
 			scsi_set_resid(cmd, scsi_bufflen(cmd) - len);
-- 
2.55.0.rc0.799.gd6f94ed593-goog

Re: [PATCH v2] ata: libata-scsi: limit simulated SCSI command copy to response length

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The defensive bounds check `WARN_ON(len > ATA_SCSI_RBUF_SIZE)` is ineffective because it evaluates after the `actor` callback has already performed an out-of-bounds write.
--

commit 41acccb75ba899e5e72a927d95a97d5d6f088c63
Author: Karuna Ramkumar <rkaruna@google.com>

ata: libata-scsi: limit simulated SCSI command copy to response length

This patch limits the sg_copy_from_buffer() size to the actual response length
instead of the full buffer size to avoid overwriting the tail of the transfer
buffer. It also adds a bounds check on the length returned by the actor
function to abort if it exceeds the static buffer capacity.

> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index d54ec1631e9a6..836981b176c7d 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1933,8 +1933,13 @@ static void ata_scsi_rbuf_fill(struct ata_device *dev, struct scsi_cmnd *cmd,
>  	memset(ata_scsi_rbuf, 0, ATA_SCSI_RBUF_SIZE);
>  	len = actor(dev, cmd, ata_scsi_rbuf);
>  	if (len) {
> +		if (WARN_ON(len > ATA_SCSI_RBUF_SIZE)) {
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Critical]
This is a pre-existing issue, but does this defensive bounds check actually
prevent memory corruption?

Because the check evaluates after the actor callback has already executed,
the actor function may have already performed an out-of-bounds write. For
example, a malicious or buggy ATA device reporting a large number of
Concurrent Positioning Ranges (nr_cpr up to 255) could cause
ata_scsiop_inq_b9() to write up to 8224 bytes into the 2048-byte statically
allocated ata_scsi_rbuf array:

drivers/ata/libata-scsi.c:ata_scsiop_inq_b9() {
	...
	for (i = 0; i < cpr_log->nr_cpr; i++, desc += 32) {
		desc[0] = cpr_log->cpr[i].num;
		desc[1] = cpr_log->cpr[i].num_storage_elements;
		put_unaligned_be64(cpr_log->cpr[i].start_lba, &desc[8]);
		put_unaligned_be64(cpr_log->cpr[i].num_lbas, &desc[16]);
	}
	...
}

Should the actor functions be modified to take a buffer size parameter so
they can safely enforce limits before writing memory?

> +			ata_scsi_set_sense(dev, cmd, ABORTED_COMMAND, 0, 0);
> +			spin_unlock_irqrestore(&ata_scsi_rbuf_lock, flags);
> +			return;
> +		}
>  		sg_copy_from_buffer(scsi_sglist(cmd), scsi_sg_count(cmd),
> -				    ata_scsi_rbuf, ATA_SCSI_RBUF_SIZE);
> +				    ata_scsi_rbuf, len);
>  		cmd->result = SAM_STAT_GOOD;
>  		if (scsi_bufflen(cmd) > len)
>  			scsi_set_resid(cmd, scsi_bufflen(cmd) - len);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702020142.3399851-1-rkaruna@google.com?part=1

Re: [PATCH v2] ata: libata-scsi: limit simulated SCSI command copy to response length

[Damien Le Moal]

On 7/2/26 11:14, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [Critical] The defensive bounds check `WARN_ON(len > ATA_SCSI_RBUF_SIZE)` is ineffective because it evaluates after the `actor` callback has already performed an out-of-bounds write.

Karuna,

Sashiko has a good point here. Let's work on fixing this with an incremental patch.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2] ata: libata-scsi: limit simulated SCSI command copy to response length

[Karuna Ramkumar]

On Wed, Jul 1, 2026 at 7:30 PM Damien Le Moal <dlemoal@kernel.org> wrote:
>
> On 7/2/26 11:14, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> >
> > Pre-existing issues:
> > - [Critical] The defensive bounds check `WARN_ON(len > ATA_SCSI_RBUF_SIZE)` is ineffective because it evaluates after the `actor` callback has already performed an out-of-bounds write.
>
> Karuna,
>
> Sashiko has a good point here. Let's work on fixing this with an incremental patch.
>
>
> --
> Damien Le Moal
> Western Digital Research

Hi Damien,

Thanks for reviewing and applying the fix patch!

I reviewed the actor functions to evaluate how to structure this
incremental patch fix.
Since the actors write to the static buffer `ata_scsi_rbuf` defined in
`libata-scsi.c` via the `rbuf` input pointer, the most robust way to
enforce bounds would be to update the actor callback signature to
accept `size_t rbuf_len` along with the `u8 *rbuf` pointer.

All the simulation actor functions (like `ata_scsiop_inq_std`,
`ata_scsiop_inq_b9`, etc) are declared static inside `libata-scsi.c`
so this change would be entirely self-contained. Each actor could
then perform bounds checks at the point of writing and return 0
(eventually failing with ABORTED_COMMAND) before an overflow
occurs.

However this refactoring will touch ~15 functions starting from the
actor callback, until all the helper functions, and result in a
~100-200 line patch.

Before I start working on this, I would love to hear your feedback on:
1. Whether you think this API update (passing `rbuf_len` down to all
actors and helper functions) is the right approach, or if you prefer
simpler, and more localized checks elsewhere.
2. What would be the best way to test all these simulated command
paths to ensure the fix is verified appropriately?

Thank you,
Karuna

Re: [PATCH v2] ata: libata-scsi: limit simulated SCSI command copy to response length

[Damien Le Moal]

On 7/7/26 08:10, Karuna Ramkumar wrote:
> Before I start working on this, I would love to hear your feedback on:
> 1. Whether you think this API update (passing `rbuf_len` down to all
> actors and helper functions) is the right approach, or if you prefer
> simpler, and more localized checks elsewhere.

I do not think it is necessary at all because rbuf is statically declared:

static u8 ata_scsi_rbuf[ATA_SCSI_RBUF_SIZE];

So the buffer address and maximum size is known to all functions using it.

> 2. What would be the best way to test all these simulated command
> paths to ensure the fix is verified appropriately?

Most functions for emulated commands generate only a very small amount of data,
far below ATA_SCSI_RBUF_SIZE. So I would suggest that we simply add checks to
the functions which may potentially exceed that. Off the top of my head, I do
not know if we have any, but we should just check.

Note that ideally, we should have each function allocate their own local buffer
to handle the translation. But this is right now a little hard to do because
these translation/emulation functions are called with the port locked, so we
would have a memory allocation for a potentially large-ish buffer under a spin
lock, which means no reclaim/no wait allocation, and thus potential for failure
which we do not have right now. A proper/ideal fix would need to drop
ata_scsi_rbuf, have some helpers for managing the internal buffer and redesign
the submission path to not have the port locked until the translation is
actually generated. That is all more work and more changes.

So as a first step, let's check the emulation function if any of them can exceed
ATA_SCSI_RBUF_SIZE in size, fail them with internal target failure.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2] ata: libata-scsi: limit simulated SCSI command copy to response length

[Damien Le Moal]

On 7/2/26 11:01, Karuna Ramkumar wrote:
> The function ata_scsi_rbuf_fill() is used to copy the response of
> emulated SCSI commands from ata_scsi_rbuf to the SCSI command's
> scatterlist.
> 
> Currently, sg_copy_from_buffer() is called with the size argument
> set to ATA_SCSI_RBUF_SIZE (2048 bytes). Since ata_scsi_rbuf is
> zeroed out before the simulation actor is invoked, copying the
> full buffer size causes the remainder of the SCSI command's
> transfer buffer (beyond the actual response length 'len') to be
> overwritten with zeroes. This clobbers any pre-existing sentinel
> values or data in the caller's buffer tail, even though the
> correct residual count is reported via scsi_set_resid().
> 
> Fix this by passing the actual response length 'len' as the copy
> size to sg_copy_from_buffer(), ensuring that the tail of the
> caller's buffer remains untouched. Also, add a defensive check
> to ensure that the actor does not return a length exceeding the
> static buffer capacity. If this occurs, trigger a WARN_ON(),
> fail the command with an aborted command error, and return
> immediately without copying any data.
> 
> The fix was tested by invoking an SCSI SG_IO INQUIRY on
> an ATA disk on vanilla build, and build with the fix. Confirmed
> that the input buffer's tail end remains unmodified with the fix.
> 
> Fixes: 5251ae224d8d ("ata: libata-scsi: Return residual for emulated SCSI commands")
> Assisted-by: Antigravity:gemini-3.5-flash
> Signed-off-by: Karuna Ramkumar <rkaruna@google.com>

Applied to for-7.2-fixes. Thanks!

-- 
Damien Le Moal
Western Digital Research