Damien Le Moal <dlemoal@xxxxxxxxxx> wrote:
>
> On 2024/09/23 13:15, Jeongjun Park wrote:
> > Damien Le Moal <dlemoal@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> >>> Tested-by: syzbot+37757dc11ee77ef850bb@xxxxxxxxxxxxxxxxxxxxxxxxx
> >>> Fixes: 602bcf212637 ("ata: libata: Improve CDL resource management")
> >>> Signed-off-by: Jeongjun Park <aha310510@xxxxxxxxx>
> >>> ---
> >>> 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.
Regards,
Jeongjun Park
>
> --
> Damien Le Moal
> Western Digital Research
