| Message ID | 20240626211358.148625-1-mish.uxin2012@yandex.ru |
|---|---|
| State | New |
| Headers | show |
| Series | [v4,5.10/5.15] ata: libata-scsi: check cdb length for VARIABLE_LENGTH_CMD commands | expand |
On 6/27/24 06:13, Mikhail Ukhin wrote: > Fuzzing of 5.10 stable branch reports a slab-out-of-bounds error in > ata_scsi_pass_thru. > > The error is fixed in 5.18 by commit ce70fd9a551a ("scsi: core: Remove the > cmd field from struct scsi_request") upstream. > Backporting this commit would require significant changes to the code so > it is bettter to use a simple fix for that particular error. This sentence is not needed in the commit message. That is a discussion to have when applying (or not) the patch. > > The problem is that the length of the received SCSI command is not > validated if scsi_op == VARIABLE_LENGTH_CMD. It can lead to out-of-bounds > reading if the user sends a request with SCSI command of length less than > 32. > > Found by Linux Verification Center (linuxtesting.org) with Syzkaller. > > Signed-off-by: Artem Sadovnikov <ancowi69@gmail.com> > Signed-off-by: Mikhail Ivanov <iwanov-23@bk.ru> > Signed-off-by: Mikhail Ukhin <mish.uxin2012@yandex.ru> Please send patches for libata to the ata maintainers (Niklas and myself). Use scripts/get_maintainer.pl And you will get our addresses and see that there is no need to spam Jens with libata patches. > --- > v2: The new addresses were added and the text was updated. > v3: Checking has been moved to the function ata_scsi_var_len_cdb_xlat at > the request of Damien Le Moal. > v4: Extra opcode check removed. > drivers/ata/libata-scsi.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index dfa090ccd21c..38488bd813d1 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -3948,7 +3948,11 @@ static unsigned int ata_scsi_var_len_cdb_xlat(struct ata_queued_cmd *qc) > struct scsi_cmnd *scmd = qc->scsicmd; > const u8 *cdb = scmd->cmnd; > const u16 sa = get_unaligned_be16(&cdb[8]); > > + if (scmd->cmd_len < 32) Given that the only service action supported is ATA_32, this check should be if (scmd->cmd_len != 32 > + return 1; > + > /* > * if service action represents a ata pass-thru(32) command, > * then pass it to ata_scsi_pass_thru handler. > -- > 2.25.1 >
On Thu, Jun 27, 2024 at 11:02:23AM +0900, Damien Le Moal wrote: >On 6/27/24 06:13, Mikhail Ukhin wrote: >> Fuzzing of 5.10 stable branch reports a slab-out-of-bounds error in >> ata_scsi_pass_thru. >> >> The error is fixed in 5.18 by commit ce70fd9a551a ("scsi: core: Remove the >> cmd field from struct scsi_request") upstream. >> Backporting this commit would require significant changes to the code so >> it is bettter to use a simple fix for that particular error. > >This sentence is not needed in the commit message. That is a discussion to have >when applying (or not) the patch. It's good to have this reasoning in the commit message to, so that later when we look at the patch and try to understand why we needed something custom for the backport, the justification will be right there.
On 6/28/24 5:08 AM, Sasha Levin wrote: > On Thu, Jun 27, 2024 at 11:02:23AM +0900, Damien Le Moal wrote: >> On 6/27/24 06:13, Mikhail Ukhin wrote: >>> Fuzzing of 5.10 stable branch reports a slab-out-of-bounds error in >>> ata_scsi_pass_thru. >>> >>> The error is fixed in 5.18 by commit ce70fd9a551a ("scsi: core: Remove the >>> cmd field from struct scsi_request") upstream. >>> Backporting this commit would require significant changes to the code so >>> it is bettter to use a simple fix for that particular error. >> >> This sentence is not needed in the commit message. That is a discussion to have >> when applying (or not) the patch. > > It's good to have this reasoning in the commit message to, so that later > when we look at the patch and try to understand why we needed something > custom for the backport, the justification will be right there. OK then, let's keep the commit message as it is. Mikhail, Please send a v5 patch with the correction I commented and the patch will be good to go. Thanks !
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index dfa090ccd21c..38488bd813d1 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -3948,7 +3948,11 @@ static unsigned int ata_scsi_var_len_cdb_xlat(struct ata_queued_cmd *qc) struct scsi_cmnd *scmd = qc->scsicmd; const u8 *cdb = scmd->cmnd; const u16 sa = get_unaligned_be16(&cdb[8]); + if (scmd->cmd_len < 32) + return 1; + /* * if service action represents a ata pass-thru(32) command, * then pass it to ata_scsi_pass_thru handler. -- 2.25.1
Fuzzing of 5.10 stable branch reports a slab-out-of-bounds error in ata_scsi_pass_thru. The error is fixed in 5.18 by commit ce70fd9a551a ("scsi: core: Remove the cmd field from struct scsi_request") upstream. Backporting this commit would require significant changes to the code so it is bettter to use a simple fix for that particular error. The problem is that the length of the received SCSI command is not validated if scsi_op == VARIABLE_LENGTH_CMD. It can lead to out-of-bounds reading if the user sends a request with SCSI command of length less than 32. Found by Linux Verification Center (linuxtesting.org) with Syzkaller. Signed-off-by: Artem Sadovnikov <ancowi69@gmail.com> Signed-off-by: Mikhail Ivanov <iwanov-23@bk.ru> Signed-off-by: Mikhail Ukhin <mish.uxin2012@yandex.ru> --- v2: The new addresses were added and the text was updated. v3: Checking has been moved to the function ata_scsi_var_len_cdb_xlat at the request of Damien Le Moal. v4: Extra opcode check removed. drivers/ata/libata-scsi.c | 3 +++ 1 file changed, 3 insertions(+)