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: <CAO9qdTFgU1TrfGTgOf=+TgwyNpk-dof2sCHr9+Ut7O+MMHaf5w@mail.gmail.com>
Date: Mon, 23 Sep 2024 20:24:48 +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

Jeongjun Park <aha310510@...il.com> 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;

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
> >