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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <5012d165-e726-e3c7-2a5a-02745dd44f3e@intel.com>
Date: Fri, 9 Feb 2024 16:20:20 -0800
From: Fenghua Yu <fenghua.yu@...el.com>
To: Lijun Pan <lijun.pan@...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

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.

Thanks.

-Fenghua