Re: [PATCH -next V2 1/2] sata_fsl: fix UAF in sata_fsl_port_stop when rmmod sata_fsl

[libaokun (A)]

在 2021/11/22 10:33, Damien Le Moal 写道:
> On 2021/11/22 11:03, libaokun (A) wrote:
> > 在 2021/11/20 20:01, Sergei Shtylyov 写道:
> > > Hello!
> > >
> > > On 20.11.2021 6:34, Baokun Li wrote:
> > >
> > > > When the `rmmod sata_fsl.ko` command is executed in the PPC64 GNU/Linux,
> > > > a bug is reported:
> > > >    ==================================================================
> > > >    BUG: Unable to handle kernel data access on read at 0x80000800805b502c
> > > >    Oops: Kernel access of bad area, sig: 11 [#1]
> > > >    NIP [c0000000000388a4] .ioread32+0x4/0x20
> > > >    LR [80000000000c6034] .sata_fsl_port_stop+0x44/0xe0 [sata_fsl]
> > > >    Call Trace:
> > > >     .free_irq+0x1c/0x4e0 (unreliable)
> > > >     .ata_host_stop+0x74/0xd0 [libata]
> > > >     .release_nodes+0x330/0x3f0
> > > >     .device_release_driver_internal+0x178/0x2c0
> > > >     .driver_detach+0x64/0xd0
> > > >     .bus_remove_driver+0x70/0xf0
> > > >     .driver_unregister+0x38/0x80
> > > >     .platform_driver_unregister+0x14/0x30
> > > >     .fsl_sata_driver_exit+0x18/0xa20 [sata_fsl]
> > > >     .__se_sys_delete_module+0x1ec/0x2d0
> > > >     .system_call_exception+0xfc/0x1f0
> > > >     system_call_common+0xf8/0x200
> > > >    ==================================================================
> > > >
> > > > The triggering of the BUG is shown in the following stack:
> > > >
> > > > driver_detach
> > > >     device_release_driver_internal
> > > >       __device_release_driver
> > > >         drv->remove(dev) --> platform_drv_remove/platform_remove
> > > >           drv->remove(dev) --> sata_fsl_remove
> > > >             iounmap(host_priv->hcr_base);            <---- unmap
> > > >             kfree(host_priv);                             <---- free
> > > >         devres_release_all
> > > >           release_nodes
> > > >             dr->node.release(dev, dr->data) --> ata_host_stop
> > > >               ap->ops->port_stop(ap) --> sata_fsl_port_stop
> > > >                   ioread32(hcr_base + HCONTROL)           <---- UAF
> > > >               host->ops->host_stop(host)
> > > >
> > > > The iounmap(host_priv->hcr_base) and kfree(host_priv) commands should
> > >    s/commands/functions/?
> > OK! I'm going to modify this in V3.
> >
> >
> > > > not be executed in drv->remove. These commands should be executed in
> > > > host_stop after port_stop. Therefore, we move these commands to the
> > > > new function sata_fsl_host_stop and bind the new function to host_stop
> > > > by referring to achi.
> > >    You mean AHCI? I don't see where you reference ahci (or achi)...
> > Yes, it's AHCI, I'm sorry for a spelling error here..
> >
> > ahci_platform_ops in drivers/ata/libahci_platform.c
> >
> >
> > > > Reported-by: Hulk Robot <hulkci@xxxxxxxxxx>
> > > > Signed-off-by: Baokun Li <libaokun1@xxxxxxxxxx>
> > >    Doesn't this need to go into the stable trees?
> >
> > I felt it was needed because the bug was triggered in a very simple way,
> >
> > although PPC linux is rare these days.
> >
> > And I will add
> >
> > Fixes: faf0b2e5afe7 ("drivers/ata: add support to Freescale 3.0Gbps SATA Controller").
> Also add:
>
> Cc: stable@xxxxxxxxxxxxxxx
>
>
> > > > ---
> > > >    drivers/ata/sata_fsl.c | 17 ++++++++++++++---
> > > >    1 file changed, 14 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
> > > > index e5838b23c9e0..30759fd1c3a2 100644
> > > > --- a/drivers/ata/sata_fsl.c
> > > > +++ b/drivers/ata/sata_fsl.c
> > > > @@ -1430,12 +1430,25 @@ static struct ata_port_operations sata_fsl_ops = {
> > > >        .pmp_detach = sata_fsl_pmp_detach,
> > > >    };
> > > >    +static void sata_fsl_host_stop(struct ata_host *host)
> > > > +{
> > > > +    struct sata_fsl_host_priv *host_priv = host->private_data;
> > > > +
> > > > +    iounmap(host_priv->hcr_base);
> > > > +    kfree(host_priv);
> > > > +}
> > > > +
> > > > +static struct ata_port_operations sata_fsl_platform_ops = {
> > > > +    .inherits       = &sata_fsl_ops,
> > > > +    .host_stop      = sata_fsl_host_stop,
> > >    Why not just add it to the initializer for sata_fsl_ops?
> >
> > This is the AHCI of the reference.
> >
> > Most ATA drivers add host_stop to to the  initializer for  xxx_platform_ops,
> >
> > such as ahci_platform_ops, ahci_brcm_platform_ops, and ahci_imx_ops.
> >
> > It feels like this separates the port operation from the host operation,
> >
> > making the hierarchy of the code clearer.
> >
> >
> > > [...]
> > >
> > > MBR, Sergei
> > > .
> >
> > Thank you very much for your advice.
> >
> > If there's nothing else to modify, I'll send a patch V3.
> >
> > --
> > With Best Regards,
> > Baokun Li
> > .

Thank you very much for your advice.

I'm about to send a patch V3 with the changes suggested by you.

--
With Best Regards,
Baokun Li
.