lkml.org 
[lkml]   [2021]   [Nov]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH -next V2 1/2] sata_fsl: fix UAF in sata_fsl_port_stop when rmmod sata_fsl
From
Date
在 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
.

\
 
 \ /
  Last update: 2021-11-22 03:51    [W:0.081 / U:0.616 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site