Re: [PATCH 1/2] ata: libata-scsi: Fix ata_msense_control_spgt2()

[Niklas Cassel <cassel@kernel.org>]

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