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 > . > -- Damien Le Moal Western Digital Research