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;
Regards,
Jeongjun Park
>
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 3328a6febc13..6ffa975746a6 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -2256,10 +2256,15 @@ static inline u16 ata_xlat_cdl_limit(u8 *buf)
> static unsigned int ata_msense_control_spgt2(struct ata_device *dev, u8 *buf,
> u8 spg)
> {
> - u8 *b, *cdl = dev->cdl->desc_log_buf, *desc;
> + u8 *b, *cdl, *desc;
> u32 policy;
> int i;
>
> + if (!(dev->flags & ATA_DFLAG_CDL) || !dev->cdl)
> + return 0;
> +
> + cdl = dev->cdl->desc_log_buf;
> +
> /*
> * Fill the subpage. The first four bytes of the T2A/T2B mode pages
> * are a header. The PAGE LENGTH field is the size of the page
>
>
> > case CDL_T2A_SUB_MPAGE:
> > case CDL_T2B_SUB_MPAGE:
> > case ATA_FEATURE_SUB_MPAGE:
> > --
>
> --
> Damien Le Moal
> Western Digital Research
>
