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

["libaokun (A)" <libaokun1@huawei.com>]

在 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
.