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

[libaokun (A)]

在 2021/11/23 2:58, Sergei Shtylyov 写道:
> Hello!
>
> On 22.11.2021 5:03, libaokun (A) 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
>
>  You should have (at least) written "the AHCI platform driver"...
>
> [...]
> > > > 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,
>
>   Most? Even if so, I guess they add it this way because they're in 
> the separate modules with the ops they inherit -- in this case it's 
> not so.
>
> > such as ahci_platform_ops, ahci_brcm_platform_ops, and ahci_imx_ops.
>
>   Note that these are all AHCI drivers, not just (more general) ATA.
>
> > It feels like this separates the port operation from the host operation,
>
>   Why separate them? The 'struct ata_port_operations' embraces many 
> different aspects of ATA, the arguments do not always include a 
> 'struct *ata_port' (I don't quite like that part in libata).
>
> > making the hierarchy of the code clearer.
>
>   Clear as mud. In your case, there's no separate modules in play, so 
> blindly parroting what the AHCI platform drivers do gives you nothing 
> but memory waste... :-(
>
> > > [...]
> > >
> > > MBR, Sergei
> > > .
> >
> >
> > Thank you very much for your advice.
>
>   You're welcome. :-)
>
> > If there's nothing else to modify, I'll send a patch V3.
>
>   Please use a single structure, it's already large enough to have 2 
> of them in the same module for no good reason.
>
> > --
> > With Best Regards,
> > Baokun Li
>
> MBR, Sergei
> .


Thank you very much for your advice.

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

--
With Best Regards,
Baokun Li