Received-SPF: pass (google.com: domain of linux-kernel+bounces-72186-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <30dee686-894b-40fb-8e4c-5f3f1922600f@intel.com>
Date: Tue, 20 Feb 2024 09:42:50 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] dmaengine: idxd: Ensure safe user copy of completion
 record
To: Fenghua Yu <fenghua.yu@intel.com>, Vinod Koul <vkoul@kernel.org>, "Dave
 Jiang" <dave.jiang@intel.com>
CC: <dmaengine@vger.kernel.org>, linux-kernel <linux-kernel@vger.kernel.org>,
	Tony Zhu <tony.zhu@intel.com>
References: <20240209191412.1050270-1-fenghua.yu@intel.com>
 <4237a933-0f61-417f-bbb6-ce5954b304d4@intel.com>
 <5012d165-e726-e3c7-2a5a-02745dd44f3e@intel.com>
Content-Language: en-US
From: Lijun Pan <lijun.pan@intel.com>
In-Reply-To: <5012d165-e726-e3c7-2a5a-02745dd44f3e@intel.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?WDltaTZCSDhGVk16eDVXV2c1TlVZRUtPbUx1aFlCb2I2azIrM21ZblczWFdS?=
 =?utf-8?B?QXFBeUppOXp5dDNWbExIcllYNnV4cTY1YVJZaVZ4U3dMWlk2QXMzVndLNUZv?=
 =?utf-8?B?RjlQTEZQM1hUMjNNNTVwMndIMjFxbnFwTENjNVJIVS9PdTVoTk1VUnN4ZzF0?=
 =?utf-8?B?WDB2M3dHRXdxVExEMnNyd0psdEgzblRnMnBxNnJTc1VJZldZdWx4QytzN0NJ?=
 =?utf-8?B?SExNNFhPTTZYR3JkNUNIS3NzNmN5R2c1UGdWNVpvQnFQSVFDcGNLTlhIR1ZL?=
 =?utf-8?B?TWxPOElReHhTVXRSNnJJRFYrM0R2TS9OZXd2b1o1bXJnMk5iRXFhbWV2U2Q2?=
 =?utf-8?B?VkJCTlUwY0FOK2haRytlK3piamJCbVdocjd3alBvU2UwSFJNVHBTYnBIeVZx?=
 =?utf-8?B?dWJtcDVQUlBQS3pubWtScVhvOERqWWtUeHJsVWloWXVJaCtncyt4NUl3UW1O?=
 =?utf-8?B?aXhxV2dCWlVSMnBBWW1DMU1HYndCNnpQWDU1REhUV3JGRERTcHlBNXBhL2Ez?=
 =?utf-8?B?QUZYWVhpRUpjSDVySWpwTlRjdTB3elUwTU1QL2RYcVN3OEZtQnVGZHhqTytF?=
 =?utf-8?B?UVV0UmhuY0dPZTdxWDVoVHorZW9jTGtORDVoVmNOQ256c29OMFZGTUU1UTMy?=
 =?utf-8?B?NGhGSGZMWGtPR3BLbnZRaHN2SmhLWTJKdFVaQ2pGdTUrUldycjlHSElmYklI?=
 =?utf-8?B?NEttek9yRnFLOTF4d3g0Y01NZldDWlM1dG9IeHRkVFprOGRRUXFIbTRXRURz?=
 =?utf-8?B?eUd5V2VIMkpvZmZ6S2JEVjdsc0pGVUlsOURUc0dFODdkVFpNZU1rNFVTaEhx?=
 =?utf-8?B?WjZUVDdvVHNmc01qZXY4YTRwRVZYK3ZNa21oaWhWTlJIYnJGN2hHUGcxTzkv?=
 =?utf-8?B?L2YzMWJHNWduWFMrWjY1aU96TkRoOHdCb2txZU1iQko5UTVFVEtXdkJWTVdJ?=
 =?utf-8?B?bnFVTStkVUszYk5neU5pTlI5WldmZDJVRjVyeHZPMDhYYzNrVmxLQldoVUUv?=
 =?utf-8?B?Y20xdGUwcnAwa0V1azBLaENINndHNDYxZEI0UEcyVE9ycTNBV25Rb2VFSnBj?=
 =?utf-8?B?TEcwRnpNSWR3QWNuZ21YTmt5ZWFIU3FPcXplUVV0OXJ1K0RsUU9TcnhLbzNM?=
 =?utf-8?B?ZEZKVWZKM2piT09kdjA3Slc1TFFRQzNEaFNIY0JZZFhjdnlBY29ZaU5qS1NJ?=
 =?utf-8?B?dHVhR3VEeDFjN29FbzZaZTEveHBZdXUycDVFQ2J3Q1BSTTF0ZG94TUU2VjB0?=
 =?utf-8?B?SXNMVTQrbk1jZ2w4c3lOMHJ2eEZjOVRDa1pXZ3FuV3ZXTy9yMkswanpPUkVn?=
 =?utf-8?B?b2dzeDVjTytoSXgzVStucncrNDliZUh1dGNNdzNYa1JOTHpjNThwa0djM1Aw?=
 =?utf-8?B?N2grS04wVmgyWS9NYkFIVkFtSXpnVXM3RGwzNVc3TUk3U3JkdW1JNXJhcUY2?=
 =?utf-8?B?MnlnMnlVa09qbnlOajJVaFFDNHFBcGVDMjI4cGpVUGJjSHdnZHpqaTNuak9u?=
 =?utf-8?B?dHJmaW1TbEM4YVlKRklTd1BZSHpZdnFoV3hKMDE3aWxkcjYwUnJVRTkxZG50?=
 =?utf-8?B?MEFaZTE2MFpxOXNMVVdmQmdYcWQ2UDN3K3hpcVI0bkl6anFrZURBaERJSVpZ?=
 =?utf-8?B?Rm8rLzYzRlRXMCtVdGt1Z1EzSnJRcnoyQ3BRaUhueTdxN21CaHFwVngrMnZj?=
 =?utf-8?B?S0pMQnJ0V0lEK0lUSEQwSEhyaUt3ZVBwTHoycHFaT1JIaDROamozclZBcTI4?=
 =?utf-8?B?M253VlFHWGhIZWQ4M0RydjhOSDFyUXFHTnhsRWJVTTBPK3JISGdrMzBXRGdU?=
 =?utf-8?B?WEJTL2tMVzI4Nm9mUmlTRFhMc1ZiOHc0Q0VKc0F3SlFWNFFrZTNqenBtV2dL?=
 =?utf-8?B?c2RlcFdmUkdueTRIZ3BSN1Yza1B3di8xc2NSSnJPRFhPSC9hZEwwSXltQjhk?=
 =?utf-8?B?OHlXaTFLM1JoKzhuSVpPZzRkOUlvOUNUSjlOR0NkVmtzUUhDL0dha3hFOW5N?=
 =?utf-8?B?b1BxRXVCOU45UDBCK2xJUzlaMExrdEUveGlDQkxqNWRTNkI4bGQyTE84NFFU?=
 =?utf-8?B?czlVYng0ckppNWQrOGU0dHNRNEdiYUtNSXBqM1RZNnFwVndSWVhQcjNQdjBw?=
 =?utf-8?Q?LIIrF4mALG/thPS7jU94X1Owc?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2d27d664-1ec9-4b5c-10d1-08dc31b54371
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB7141.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Feb 2024 01:42:57.3175
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 1oPm+wgZeFFcbUyVm0ch+lAwBsVMoX+LU0UbrW1G+nBKLdWitmdnl9zJ6pcWX/JoRxM71CX4VbAHDMI2gGT4tw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR11MB5351
X-OriginatorOrg: intel.com


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@intel.com>
>>> Tested-by: Tony Zhu <tony.zhu@intel.com>
>>> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
>>
>> Reviewed-by: Lijun Pan <lijun.pan@intel.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