[PATCH v4 5.10/5.15] ata: libata-scsi: check cdb length for VARIABLE_LENGTH_CMD commands

[PATCH v4 5.10/5.15] ata: libata-scsi: check cdb length for VARIABLE_LENGTH_CMD commands

[Mikhail Ukhin]

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(+)

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

Re: [PATCH v4 5.10/5.15] ata: libata-scsi: check cdb length for VARIABLE_LENGTH_CMD commands

[Damien Le Moal]

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
> 

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4 5.10/5.15] ata: libata-scsi: check cdb length for VARIABLE_LENGTH_CMD commands

[Sasha Levin]

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.

-- 
Thanks,
Sasha

Re: [PATCH v4 5.10/5.15] ata: libata-scsi: check cdb length for VARIABLE_LENGTH_CMD commands

[Damien Le Moal]

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 !

-- 
Damien Le Moal
Western Digital Research