linux-kernel - Re: [PATCH] dmaengine: idxd: Ensure safe user copy of completion record

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 20 Feb 2024 09:42:50 +0800
From: Lijun Pan <lijun.pan@...el.com>
To: Fenghua Yu <fenghua.yu@...el.com>, Vinod Koul <vkoul@...nel.org>, "Dave
 Jiang" <dave.jiang@...el.com>
CC: <dmaengine@...r.kernel.org>, linux-kernel <linux-kernel@...r.kernel.org>,
	Tony Zhu <tony.zhu@...el.com>
Subject: Re: [PATCH] dmaengine: idxd: Ensure safe user copy of completion
 record



On 2/10/2024 8:20 AM, Fenghua Yu wrote:
> Hi, Lijun,
> 
> On 2/9/24 13:53, Lijun Pan wrote:
>>
>>
>> On 2/10/2024 3:14 AM, Fenghua Yu wrote:
>>> If CONFIG_HARDENED_USERCOPY is enabled, copying completion record from
>>> event log cache to user triggers a kernel bug.
> 
> ...
> 
>>> Fixes: c2f156bf168f ("dmaengine: idxd: create kmem cache for event 
>>> log fault items")
>>> Reported-by: Tony Zhu <tony.zhu@...el.com>
>>> Tested-by: Tony Zhu <tony.zhu@...el.com>
>>> Signed-off-by: Fenghua Yu <fenghua.yu@...el.com>
>>
>> Reviewed-by: Lijun Pan <lijun.pan@...el.com>
>>
>>> ---
>>>   drivers/dma/idxd/init.c | 15 ++++++++++++---
>>>   1 file changed, 12 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
>>> index 14df1f1347a8..4954adc6bb60 100644
>>> --- a/drivers/dma/idxd/init.c
>>> +++ b/drivers/dma/idxd/init.c
>>> @@ -343,7 +343,9 @@ static void idxd_cleanup_internals(struct 
>>> idxd_device *idxd)
>>>   static int idxd_init_evl(struct idxd_device *idxd)
>>>   {
>>>       struct device *dev = &idxd->pdev->dev;
>>> +    unsigned int evl_cache_size;
>>>       struct idxd_evl *evl;
>>> +    const char *idxd_name;
>>>       if (idxd->hw.gen_cap.evl_support == 0)
>>>           return 0;
>>> @@ -355,9 +357,16 @@ static int idxd_init_evl(struct idxd_device *idxd)
>>>       spin_lock_init(&evl->lock);
>>>       evl->size = IDXD_EVL_SIZE_MIN;
>>> -    idxd->evl_cache = kmem_cache_create(dev_name(idxd_confdev(idxd)),
>>> -                        sizeof(struct idxd_evl_fault) + 
>>> evl_ent_size(idxd),
>>> -                        0, 0, NULL);
>>> +    idxd_name = dev_name(idxd_confdev(idxd));
>>> +    evl_cache_size = sizeof(struct idxd_evl_fault) + 
>>> evl_ent_size(idxd);
>>> +    /*
>>> +     * Since completion record in evl_cache will be copied to user
>>> +     * when handling completion record page fault, need to create
>>> +     * the cache suitable for user copy.
>>> +     */
>>
>> Maybe briefly compare kmem_cache_create() with 
>> kmem_cache_create_usercopy() and add up to the above comments. If you 
>> think it too verbose, then forget about it.
> 
> It's improper to add comment to compare the two functions here because:
> 1. When people look into this code in init.c, they have no idea why 
> compare the functions here when only kmem_cache_create_usercopy() is 
> used. The comparison is only meaningful in this patch's context and has 
> been explained in the patch commit message.
> 
> 2. Comparison or any details of the function can be found easily in the 
> function implementation. No need to add more details on top of the 
> current comment which covers enough info (i.e. why call this function) 
> already.
> 
> Given the above reasons, I will keep the current comment and patch 
> without change.
> 


That's fine.

Lijun