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

[Karuna Ramkumar]

Thanks for the feedback! I'll push a new patch failing the command
instead of the warning.

Thanks,
Karuna Ramkumar

On Mon, Jun 29, 2026 at 5:05 PM Damien Le Moal <dlemoal@xxxxxxxxxx> wrote:
>
> On 6/30/26 02:50, 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
> > WARN_ON() check to ensure that the actor does not return a
> > length exceeding the static buffer capacity.
> >
> > 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@xxxxxxxxxx>
> > ---
> >  drivers/ata/libata-scsi.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index d54ec1631e9a..0ddec111a78c 100644
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> > @@ -1933,8 +1933,10 @@ 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))
> > +                     len = ATA_SCSI_RBUF_SIZE;
>
> Yes, it is good to add a warning, but if this ever happen, then it is a serious
> bug in libata-scsi and we should fail the command rather than return data from
> something buggy. So let's do that rather than just warn.
> The other change below looks good.
>
> >               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);
>
>
> --
> Damien Le Moal
> Western Digital Research