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 | Tue, 23 Nov 2021 09:11:52 +0800 |
| |
在 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
| |