linux-kernel - Re: [PATCH] ata: libata: fix ALL_SUB_MPAGES not to be performed when CDL is not supported

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAO9qdTE+vdFPsCLfaxMK1N8t=9+zSF88LQr-C5L=K9rZRoQjSQ@mail.gmail.com>
Date: Mon, 23 Sep 2024 22:22:13 +0900
From: Jeongjun Park <aha310510@...il.com>
To: Damien Le Moal <dlemoal@...nel.org>
Cc: cassel@...nel.org, syzbot+37757dc11ee77ef850bb@...kaller.appspotmail.com, 
	linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ata: libata: fix ALL_SUB_MPAGES not to be performed when
 CDL is not supported

Damien Le Moal <dlemoal@...nel.org> wrote:
>
> On 2024/09/23 13:15, Jeongjun Park wrote:
> > Damien Le Moal <dlemoal@...nel.org> 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@...kaller.appspotmail.com
> >>> Tested-by: syzbot+37757dc11ee77ef850bb@...kaller.appspotmail.com
> >>> Fixes: 602bcf212637 ("ata: libata: Improve CDL resource management")
> >>> Signed-off-by: Jeongjun Park <aha310510@...il.com>
> >>> ---
> >>>  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