Re: [PATCH] ata: libata: fix ALL_SUB_MPAGES not to be performed when CDL is not supported

[Damien Le Moal <dlemoal@kernel.org>]

On 2024/09/23 15:22, Jeongjun Park wrote:
> Damien Le Moal <dlemoal@kernel.org> wrote:
>>
>> On 2024/09/23 13:15, Jeongjun Park wrote:
>>> Damien Le Moal <dlemoal@kernel.org> wrote:
>>>>
>>>> On 2024/09/21 14:41, Jeongjun Park wrote:
>>>>> In the previous commit 602bcf212637 ("ata: libata: Improve CDL resource
>>>>> management"), the ata_cdl structure was added and the ata_cdl structure
>>>>> memory was allocated with kzalloc(). Because of this, if CDL is not
>>>>> supported, dev->cdl is a NULL pointer, so additional work should never
>>>>> be done.
>>>>>
>>>>> However, even if CDL is not supported now, if spg is ALL_SUB_MPAGES,
>>>>> dereferencing dev->cdl will result in a NULL pointer dereference.
>>>>>
>>>>> Therefore, I think it is appropriate to check dev->flags in
>>>>> ata_scsiop_mode_sense() if spg is ALL_SUB_MPAGES to see if CDL is supported.
>>>>>
>>>>> Reported-by: syzbot+37757dc11ee77ef850bb@syzkaller.appspotmail.com
>>>>> Tested-by: syzbot+37757dc11ee77ef850bb@syzkaller.appspotmail.com
>>>>> Fixes: 602bcf212637 ("ata: libata: Improve CDL resource management")
>>>>> Signed-off-by: Jeongjun Park <aha310510@gmail.com>
>>>>> ---
>>>>>  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 3328a6febc13..6f5527f12b0e 100644
>>>>> --- a/drivers/ata/libata-scsi.c
>>>>> +++ b/drivers/ata/libata-scsi.c
>>>>> @@ -2442,7 +2442,9 @@ static unsigned int ata_scsiop_mode_sense(struct ata_scsi_args *args, u8 *rbuf)
>>>>>       if (spg) {
>>>>>               switch (spg) {
>>>>>               case ALL_SUB_MPAGES:
>>>>> -                     break;
>>>>> +                     if (dev->flags & ATA_DFLAG_CDL)
>>>>> +                             break;
>>>>> +                     fallthrough;
>>>>
>>>> I do not think this is correct at all. If the user request all sub mpages, we
>>>> need to give that list regardless of CDL support. What needs to be fixed is that
>>>> if CDL is NOT supported, we should not try to add the information for the T2A
>>>> and T2B sub pages. So the fix should be this:
>>>
>>> Okay. But after looking into it further, I think it would be more appropriate to
>>> also check the ATA_DFLAG_CDL_ENABLED flag when checking if CDL is
>>> not supported. So it seems like it would be better to modify the condition as
>>> below.
>>>
>>> What do you think?
>>>
>>> if (!(dev->flags & ATA_DFLAG_CDL
>>>       dev->flags & ATA_DFLAG_CDL_ENABLED) || !dev->cdl)
>>>         return 0;
>>
>> No, that would be wrong. The mode sense is to report if CDL is *supported*, not
>> if it is enabled or not. So we always must report the T2A and T2B pages for SATA
>> drives that support CDL, even if the CDL feature is disabled.
>>
>> The flag ATA_DFLAG_CDL_ENABLED is not checked in ata_scsiop_mode_sense() for
>> this reason. Adding that check in ata_msense_control_spgt2() would be wrong.
>>
> 
> Thanks for your reply! I will write a v2 patch to meet the
> requirements you provided
> and send it to you.

No need. I have the fix patch already. And while writing it, I also noticed that
ata_msense_control() is wrong (the ALL_SUB_MPAGES case adds the T2A page twice
instead of adding the T2A page and then the T2B page. So I have another patch to
fix that.

Will be posting soon.

> 
> Regards,
> Jeongjun Park
> 
>>
>> --
>> Damien Le Moal
>> Western Digital Research


-- 
Damien Le Moal
Western Digital Research