Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
Message-ID: <3f033585-91c6-1d70-be1e-f083fc855aed@amd.com>
Date:   Sat, 25 Nov 2023 09:10:09 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: [PATCH] crypto: ccp - fix memleak in ccp_init_dm_workarea
Content-Language: en-US
To:     Dinghao Liu <dinghao.liu@zju.edu.cn>
Cc:     John Allen <john.allen@amd.com>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        "David S. Miller" <davem@davemloft.net>,
        linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20231123070337.11600-1-dinghao.liu@zju.edu.cn>
From:   Tom Lendacky <thomas.lendacky@amd.com>
In-Reply-To: <20231123070337.11600-1-dinghao.liu@zju.edu.cn>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?QWVQbXNBMnVsWmloWjVYaWtTczVqNVFGTTh1eFlOcjEzQ0dTNGc1YU0rdm50?=
 =?utf-8?B?NkZqRGh2alg4TS81dzd5TmxQVXRKTTFiN2V3REN1Q3F6TWRRVmJBaWpQSHdn?=
 =?utf-8?B?Zml3c0NjSy8yWUIya2Nscnpwb3M0TzJ6d1FVNllWSVhkbEZWOTQvRVhoMVk1?=
 =?utf-8?B?a2NnSURRUEMvSFZIakV4T1RVVTBIQVRiekVTTTdmYlFXekFFQUF5WXRZT0pz?=
 =?utf-8?B?L3IvRzVPOXZpbHJGdmZaM3BlQ21ndjh3enhhTDdmTDk1ZTkvRWdrNGVmMk55?=
 =?utf-8?B?S2dvcFFNa0pFd1drYXE3UTFWd0ROMmlpem81SUcrQVJkR0h2NGg3ZUNLWHcy?=
 =?utf-8?B?elRUMkQzNTZjWHNlcmdSWlU2ZmR4d21aeDZXUDBQU1ZWSnp4Y1c1QjZEUlhY?=
 =?utf-8?B?UkdaL1JrY2ZXMW9oZ0ZNMmJzMFNCb1N6VEZTb2pDRjNaQmNoam95NzNUelJQ?=
 =?utf-8?B?MWQyWHVjYUJLTTNvd29JRXpHOGFhMEt5d2xPRUh1alZmblREbGVKUXdURDdC?=
 =?utf-8?B?Z2xCR1BhRzY1TTBlRDRoNS82Njl3NUFRVWxlZnN0YlZZY05EWTNCcFNyL0hm?=
 =?utf-8?B?SmYxb2V0RklnVTlMTUNURXk1QWxJVzV6Y0Z5Uks0YzR3TG9wMU5LejI4emx3?=
 =?utf-8?B?RDlDQ21uLzY0YytPNEl5OXIzVkJzUkFVbnBjcFkvWFRKR3VQYy9xZ0w1WG90?=
 =?utf-8?B?ZEk0OFMvYkNYNXNYWVNRajVJZ3FwQ3NrUmlaNjBUdEk3UGV4NTZtcm96NmZ4?=
 =?utf-8?B?UU9tMGhIWUpwcFFja0RDdlg3aW0zUXV0dHJERG0xYWxVYm1KMjZUUlZCa2Rv?=
 =?utf-8?B?Z3Q4U2tvRGplMFpXMlRWdWNzR2tzMUVCYnVseWR3RVMwandpMGRxSzVpTG1u?=
 =?utf-8?B?cDJFOXBCZG9qWkJPMzFZSWtKVmRjdGs0R2JpOENVVXVkaVlDUEluQlo5WlFu?=
 =?utf-8?B?ZnpXZzB1K3pDR0xIeTB5dXh5K29JaHRYSHZaWGoxWEhLUThCczlZakhGNmRq?=
 =?utf-8?B?ekFCOUJ6Sk1BVEFneXFQR0FWVCtyTWQ4Q1J3SFJ2QWxkakpINnVFMnNhNnFX?=
 =?utf-8?B?RlVQTzFNR0FHSjBDS3ZvNUxvYlpzK3J3L2hQQVhva1BOclhEaGU5WFlBbkw1?=
 =?utf-8?B?TTVkaDdFSXFReDVDV0t6Ui9Lc0NHYVFPUWtVdTB4czRVSG5KOTdjRi9DNE9y?=
 =?utf-8?B?UWsyTVA3K2tvM0EwK1UrakZSZCs1ckwvWFZJRExxY01kUDB3eVFWZzJzcVZN?=
 =?utf-8?B?YUJjUjZLUko5RndOYTBoVThxdUFVY0RRMkZPVXBucHQrZURiV1lKZTBYcmJw?=
 =?utf-8?B?a21tSExHRVFBVHVDT2E3RCtoSXZ1YUVzMGRoa2pzZXc4WXlhSjI1Y3BaNlgy?=
 =?utf-8?B?MVg3N3VFdkhweUc3NHRSSkNyeExZUisvanV1SU9ZUTZEdnJ4cWVtT2NQV1Rv?=
 =?utf-8?B?Q283VHZoa2pCNStaN0d2ek5Fd2tnMWY4RUhrTlJ5ZDR4dzl4cnNFVTh5Z2tu?=
 =?utf-8?B?WEdGc3FtM2NZMWlJVjc4dmdjUnZxV0NVQ0o3Y0JyYmNQNHdDVXpya0xaRVQ3?=
 =?utf-8?B?VEg0cGVIUHpXUnFWVVZtdHVtL2p4VndVN2dFb2lsTGJLeURVQS9scWZkS0J5?=
 =?utf-8?B?SlM1aWxFWk5pQlowVnNEbjBzdEdxYzdZVDFhbHVXTUgxcytLNjBJUEtaOEVO?=
 =?utf-8?B?dit1YzRIRGNrQUZHM1VUZWdGSDJwUWlOb3g5dTlRVjhiU05LUW9zVmlTMm1G?=
 =?utf-8?B?b1NpSjhIU1FWYzkyeUt0L1lZSFJub2VoYkY4WW5JcFNhbWIvVzJoUnB3WHFt?=
 =?utf-8?B?US9mQzdITXduRGdhZmhBeFYwNVJHUGZTOFUrcE1JTVRYQVBhOW85NWVVYVpS?=
 =?utf-8?B?a0VNTjVoUDdJdy9kbzB3NlFWSFRTYUtKMHZGQWZqenQwWDdiN20wSG4xVVlz?=
 =?utf-8?B?dXUxbVFLTnBoWDhPTmJHWUVZN28yVlorMWFYNG9yMjN2NS90aGtRdE5CMmow?=
 =?utf-8?B?TVQvc3Y4NGhRZjBYVndSVGNUNFBDVnVDV0F5L3Y5NmVOWG9ORUxZVDVaL1VF?=
 =?utf-8?B?b2tHQlRxVGsvS2w2UG43SjllSjlrRGVYRDNIM2E4U1dueVJjdGJVaUFVV096?=
 =?utf-8?Q?sm1a32f5LOrlD9BaUVAN4BOcS?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d4a3699a-38a3-4f23-183d-08dbedc89edf
X-MS-Exchange-CrossTenant-AuthSource: BL1PR12MB5732.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Nov 2023 15:10:11.8985
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: iFvNNc9xuJdaws5yW/1zsisfDBnxdUqqmodSTRVQROxWSPUU+5jyjZWSX0hjiH9veGHpCd89rVXwjabH2sU1gw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH3PR12MB9343
X-Spam-Status: No, score=-3.5 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,
	T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Sat, 25 Nov 2023 07:10:25 -0800 (PST)

On 11/23/23 01:03, Dinghao Liu wrote:
> When dma_map_single() fails, we should free wa->address to
> prevent potential memleak.

You should expand on this a bit more. The ccp_dm_free() function is 
normally called to free that memory, but many of the call spots don't 
expect to have to call ccp_dm_free() on an error return from 
ccp_init_dm_workarea(). Because of that, the memory is freed in 
ccp_init_dm_workarea().

So a more detailed commit message about why this change is needed should 
be provided.

> 
> Fixes: 63b945091a07 ("crypto: ccp - CCP device driver and interface support")
> Signed-off-by: Dinghao Liu <dinghao.liu@zju.edu.cn>
> ---
>   drivers/crypto/ccp/ccp-ops.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/crypto/ccp/ccp-ops.c b/drivers/crypto/ccp/ccp-ops.c
> index aa4e1a500691..45552ae6347e 100644
> --- a/drivers/crypto/ccp/ccp-ops.c
> +++ b/drivers/crypto/ccp/ccp-ops.c
> @@ -179,8 +179,10 @@ static int ccp_init_dm_workarea(struct ccp_dm_workarea *wa,
>   
>   		wa->dma.address = dma_map_single(wa->dev, wa->address, len,
>   						 dir);
> -		if (dma_mapping_error(wa->dev, wa->dma.address))
> +		if (dma_mapping_error(wa->dev, wa->dma.address)) {
> +			kfree(wa->address);

If future changes should result in ccp_dm_free() being called on an error 
returned from ccp_init_dm_workarea(), you should add:

			wa->address = NULL;

Thanks,
Tom

>   			return -ENOMEM;
> +		}
>   
>   		wa->dma.length = len;
>   	}