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 >