| Message ID | 20240923133949.286295-2-dlemoal@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | CDL related fixes | expand |
On 9/23/24 15:39, Damien Le Moal wrote: > ata_msense_control_spgt2() can be called even for devices that do not > support CDL when the user requests the ALL_SUB_MPAGES mode sense page, > but for such device, this will cause a NULL pointer dereference as > dev->cdl is NULL. Similarly, we should not return any data if > ata_msense_control_spgt2() is called when the user requested the > CDL_T2A_SUB_MPAGE or CDL_T2B_SUB_MPAGE pages for a device that does not > support CDL. > > Avoid this potential NULL pointer dereference by checking if the device > support CDL on entry to ata_msense_control_spgt2() and return 0 if it > does not support CDL. > > 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: Damien Le Moal <dlemoal@kernel.org> > --- > drivers/ata/libata-scsi.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index 061fe63497bf..97c84b0ec472 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -2245,10 +2245,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 Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
On Mon, Sep 23, 2024 at 10:39:48PM +0900, Damien Le Moal wrote: > ata_msense_control_spgt2() can be called even for devices that do not > support CDL when the user requests the ALL_SUB_MPAGES mode sense page, > but for such device, this will cause a NULL pointer dereference as > dev->cdl is NULL. Similarly, we should not return any data if > ata_msense_control_spgt2() is called when the user requested the > CDL_T2A_SUB_MPAGE or CDL_T2B_SUB_MPAGE pages for a device that does not > support CDL. > > Avoid this potential NULL pointer dereference by checking if the device > support CDL on entry to ata_msense_control_spgt2() and return 0 if it > does not support CDL. > > Reported-by: syzbot+37757dc11ee77ef850bb@syzkaller.appspotmail.com > Tested-by: syzbot+37757dc11ee77ef850bb@syzkaller.appspotmail.com > Fixes: 602bcf212637 ("ata: libata: Improve CDL resource management") Looks good, but the commit above also changes ata_eh_get_ncq_success_sense(), and also adds calls to free the resources in ata_dev_free_resources(), which is called by different EH paths, so perhaps we also need a: if (!dev->cdl) guard in ata_eh_get_ncq_success_sense() ? EH is a bit complex, but it would make sense if EH already ensures that ata_eh_get_ncq_success_sense() can't be called after ata_dev_free_resources() has been called. In that case, we shouldn't need to add an additional guard. Kind regards, Niklas
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 061fe63497bf..97c84b0ec472 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -2245,10 +2245,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