Received-SPF: pass (google.com: domain of linux-kernel+bounces-71746-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <82d6f21b-3bc6-2094-385c-f9ff849203dc@intel.com>
Date: Mon, 19 Feb 2024 09:20:23 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.11.0
Subject: Re: [PATCH] dmaengine: idxd: Ensure safe user copy of completion
 record
Content-Language: en-US
From: Fenghua Yu <fenghua.yu@intel.com>
To: Lijun Pan <lijun.pan@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>
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?Q0N5M002SUNYb0pFOFl5WDNCSUVDZ2JadmVkVUp6QWRUYUw4Q2c3OUNxVEky?=
 =?utf-8?B?TkpkdUhSYzJSSmo3Q3hLUnpGWjRPMW0vNituZXl2MTcyZmU3Z0NxUi9JMmpE?=
 =?utf-8?B?bTdhMnlEL2hLbEZtOFZJVTFOeUdSS3lPeUpxNHJqaGJuOFVLUWtBbnFaNFda?=
 =?utf-8?B?aE16L3dDc2hSakN0d2pmYzgwQnAydURFWlhzaXRYVGtvc0F3QzNQcEVDRU92?=
 =?utf-8?B?TklQVWZvTkZkbXV5MTU3SUpxaWUwV1VVV0VTNnhhVjFDQ01EWEhSUFA5RWV2?=
 =?utf-8?B?RGVCU01nQTNSTExOMVM2SFNZNkFWaitvZzdsTHNsNlZOTmU2OWhxNVYreVR1?=
 =?utf-8?B?K1dKaS91ZXpLNDNpU1dqTHl1dHNpb1k5VTlobFRhSkNvcDM0YU4rZVEyN2pv?=
 =?utf-8?B?eGVpWk9NRUE3TG5kTU9UNE9vSVV6NFJtdERkeTFqSkNGa2VnL2xXd1kxdDdj?=
 =?utf-8?B?OTdhNkJmUUF5cFNxcFJWNXQzR3phMldvN1RLTzltYWZFMWVKd1UvQjkySmto?=
 =?utf-8?B?UnpsU2xFUlBYWldVUDlUZWs5a01hcC9OcjF6bzhzeXhWVTBUcGlNZGhUV002?=
 =?utf-8?B?MHI5NVhzSmNNbGxsQzkyUXcxMy9GamFNUVpkMmRIUXd3dEE0ZDhyeHdtK0or?=
 =?utf-8?B?am9ncVNYajViM2tDbk96a1owb2xVNE12SEhHWEZNSkRpU1N1ZnlCRlJDK1Qz?=
 =?utf-8?B?dXNYSHRvWm90RUdpZ0ZKcWlnSkh0WXhQeWhCb25pZnhyNytBTE1MbCtVUFpw?=
 =?utf-8?B?bzBSRmtiZmVuUWxlbEFqZjh6QlFoZHZERHVxTXFkRUlITmozaEllWkdBN0xl?=
 =?utf-8?B?R3lXcExweWVJdHlteHVnOHhYL2oyNTVla3o1TXVFUTd1NzlNOVVNd1NWbFlW?=
 =?utf-8?B?UExVZmdoRmJHcWtZVlNvVHhpQjJwZ1E0aGcrLytWOWxKTFprdnMvYjh4Yndp?=
 =?utf-8?B?ZWplQmNzMEI0c0hCTGNVZTUvM1kxN2VnYndMTWpNdkxQbG1SZzNCcWlhWWgr?=
 =?utf-8?B?TnloU280UFVDdXF1M1BqelcrQ3ZEcysxa1lPQTFaZUVmMGVwWHdZc0c1WmlL?=
 =?utf-8?B?Rjl4NmFjZ09TdVZ5TEZkcjNOWWhDTmZhb2RDNExkSkg1ekpXaGRjRnRhVHo3?=
 =?utf-8?B?dzhNSFc5MlM1ZElrNDJJdXhDTlNLdUFDVzVMTi9sU3NjTzBEeEUybUNhakRQ?=
 =?utf-8?B?YjF3aTB0Z3o1T0Rua050UnI2aUxiMis5bGlReG56M29SRmRmVWdWN3Zod0JD?=
 =?utf-8?B?ZmdramZ2TSt0ZlMydEswMGwyUkplOTVLQVcxUVFsaXhZcmZrQ0FsRmkrY05y?=
 =?utf-8?B?QlJkbm9CdENheXdZWEV1SFpoT2dGcUQ1bHRMaGRjZUtoNUMvVHhqWjgvQVdX?=
 =?utf-8?B?WklRTElOSGJxWmEvcVFpMWwxcmpXM3g4OHBjdnBxYnc2alZwcDVPbCtka1lz?=
 =?utf-8?B?NlNNTkJScE50K0NaMkxPQ0tNb1RtOVErVUVDTldIbitSNFc2RTZ5ZHg5Y3RJ?=
 =?utf-8?B?M0ZuTjZkTFRmZFFaZTZXSWJSTng5d3lhWVdJdDU4WFZQYWRUNHhkUkl6Z20w?=
 =?utf-8?B?QjF0YWhDUUF6UnBMeVFUbWszc1JSb2Z2bDJhSDJGdEZIc1Z3b1Q4WUNMOTdw?=
 =?utf-8?B?L3B2NE1qNThsNUhrS0J1bG9jSTJwNnJRRHZjYXJJZ2RCZDZXaC81SHZQS05p?=
 =?utf-8?B?YjJ3YlNud2JvVkNHQjBiNWh4N2tPU2pjTTRuZnJuRVNHcHhsMkRnTWNsQnpj?=
 =?utf-8?B?OVNOMU4rcEljUDhuTlN6OWJxckVkeCtlL20rOGV2cVpLTmJKWENIZ0dLb3JP?=
 =?utf-8?B?VEk0c2xXa0JLUEx1YXRqVWtuamEwWFMrck5NMlpFQW1EU2dvdXpMb0dHUUwy?=
 =?utf-8?B?Zzk5ZFl4OWFCMUcrVi9zZTdQWDN0RHhrNytCbUtMbU1TcWRiSGw4VisrS0hI?=
 =?utf-8?B?ZG44akRyV0E5bHdBblRoc1U3U1lVLzFQYjR1K0JjUXA0MVZ3dU8xZmhsOUZy?=
 =?utf-8?B?Y2hMTzNqQ0NGK01vS1ltaTdxcXZhR2RGeXJSeGxEc1VTZG5hS2Y1dUtzK2ha?=
 =?utf-8?B?LzBXaVlBYVNsTk9LTGRnRFI5R1dzbVo2VHdpYmdFVkZFL0J1dWdPNVhYZ2t5?=
 =?utf-8?Q?scN+3Z2npsTNK9xJ4xdnkA9MI?=
X-MS-Exchange-CrossTenant-Network-Message-Id: cbf2e7d6-8ff3-4057-39ff-08dc316f1ebb
X-MS-Exchange-CrossTenant-AuthSource: IA1PR11MB6097.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Feb 2024 17:20:50.8469
 (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: OkUl9U5weGQJRF2CMnS6dYS8lpq0MP25gEEalM51u7H97mnzy01gQnx2s/IobCHY1ZAtmAshlUW4b4w53657vw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR11MB5482
X-OriginatorOrg: intel.com

Hi, Vinod,

On 2/9/24 16:20, 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.

Since Dave gave Reviewed-by already and the comment from Lijun is 
invalid and won't be addressed, could you please apply this patch to 
dmaengine tree for upstream inclusion?

Thank you very much!

-Fenghua