Messages in this thread | | | Subject | Re: [PATCH -next V2 1/2] sata_fsl: fix UAF in sata_fsl_port_stop when rmmod sata_fsl | From | "libaokun (A)" <> | Date | Mon, 22 Nov 2021 10:50:33 +0800 |
| |
在 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@huawei.com> >>>> Signed-off-by: Baokun Li <libaokun1@huawei.com> >>> 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@vger.kernel.org > > >>>> --- >>>> 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 .
| |