Received-SPF: pass (google.com: domain of linux-crypto+bounces-4220-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Thu, 16 May 2024 22:16:48 +0100
From: "Cabiddu, Giovanni" <giovanni.cabiddu@intel.com>
To: Herbert Xu <herbert@gondor.apana.org.au>
CC: Damian Muszynski <damian.muszynski@intel.com>,
	<linux-crypto@vger.kernel.org>, <qat-linux@intel.com>
Subject: Re: [PATCH] crypto: qat - Fix ADF_DEV_RESET_SYNC memory leak
Message-ID: <ZkZ3wLoEN6qCuf6q@gcabiddu-mobl.ger.corp.intel.com>
References: <20240209124403.44781-1-damian.muszynski@intel.com>
 <Zjs6VxtkL8QLtHIH@gondor.apana.org.au>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <Zjs6VxtkL8QLtHIH@gondor.apana.org.au>
Organization: Intel Research and Development Ireland Ltd - Co. Reg. #308263 -
 Collinstown Industrial Park, Leixlip, County Kildare - Ireland
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?HaQgo4rmmFMBamMZu28XjTXPlqPS/oRg9b0a81CUz6uGOBKp7JJwp+afsZgv?=
 =?us-ascii?Q?pkZcPln1H+GZsmME9DD/ldi7yQ7e5EosATEp/2EHKjUraxhA/sCvrfaqsK90?=
 =?us-ascii?Q?beep3pq3IXzS5EbY67nvd25LPFfAvzyYwP9iCgSLfucxT4Rh+DeGpsEDQtyG?=
 =?us-ascii?Q?/VVykEjNBzm5fMhBa0z7FyGQand3L2Atlek+Ez0lJCuDOVfDXozifVGgr63M?=
 =?us-ascii?Q?ah1B5sukA/epz7FjU5+sfyrtR2pqT2cUgU1I3e3xWUl4rNfH5+JCeIuz0wPn?=
 =?us-ascii?Q?ozBp3ADjaHkcBe8IyqmY2H1yIdqa1WpSSVo3mWN785Ia7YyLp+6ekgyTFVy8?=
 =?us-ascii?Q?onIjrunJhRryr5MSdlPsRmY9+EYnCZq+NQ4CBI7ahxjgEVG/+4Q1QoTP7F1a?=
 =?us-ascii?Q?FS1FusxmNAJbHoR28t5asFj62SHIz15gzZ5/IXuobF+gLXzZ3Yru9tGC9EUz?=
 =?us-ascii?Q?M/i5KU1A7YE91oeIeepgh33DlC3oyllzjeZrihSo0bd4M3NKohbZ9AWq0WNd?=
 =?us-ascii?Q?X8AC7WMs0vGXi/dxWBL5PxqoRyGtDrIlBi/3XC3xhWH+8HGAXRZ7LLK8ANT/?=
 =?us-ascii?Q?pNJ5KK6aNy1wQHLhhjv4p9qvFY6y+q4gm5V9PPeHgP4x3pMhMv4KSvI7qUsi?=
 =?us-ascii?Q?HbguP36lVD3CXEvFVQ48hDj4I6EphXDUtVJC8Sj2pVoRV0eS1rFftQggW2Z5?=
 =?us-ascii?Q?AaGaXARAELQG0mGHVzDUcc5zqJ7DPuKOUvxSljN6uNydFGvuke4E1l1WHpnA?=
 =?us-ascii?Q?khxnBGaVNE0fOjFmLGThpc7I+MVUVWxMlIWyqlDv9mHhCU4QD57slsiwmDiW?=
 =?us-ascii?Q?p/ZMvtoS9akzNz4RizxV42f2xf8J/RpXkacb0FsK3U1m3gLuHVct6V9SnfFq?=
 =?us-ascii?Q?M1Xo4y+C6tNahkurJ5E4nusiL6qlhHwcewCJzjodDuDJwujB0fHZ7XtYwI0W?=
 =?us-ascii?Q?zPPUz/IkqIWxd91oV7XQ3hEbFGGGNLg8EBB2xSBRtpp+ZTJn+eVHnOafjtpY?=
 =?us-ascii?Q?bsKZVMtKZ6xUsbtPyBBXV+cIhP7VY5i476z3TyOe2ZOO8jDepKEAzkt9spWB?=
 =?us-ascii?Q?zoKzPjT81z1lMTUMQUnHKAkOjDGnZUVId89lOCw81uGN1PyeeLtX2YM5jdVa?=
 =?us-ascii?Q?nLKi9fzGzvuDZaCd2ZMu7FzLCUoUiM0hneFbMC9ccvZxhkS9MtvB93PUCawS?=
 =?us-ascii?Q?0cJl7GsIfeNOGx4HahGCly1xFw2+RP2v3eLq4uMsjZkKH2uGAt02N+IiHoo/?=
 =?us-ascii?Q?h9PbXwTt2xeSdtoMRy2vNB2+ZVW37hTHG+mHqd9qe+l7+jAfTaZVojRQiC1O?=
 =?us-ascii?Q?rWdFiExC5bAkvKf17qZaz/0pIa4BjCmWs/Piig6y4yR24SNCJx89Wo6z5XXq?=
 =?us-ascii?Q?t/YSkekQOyipBKVC5crdFZ64b/so6JjPEnxtuNBs6rpLOVU7tZjcMK3uYoGQ?=
 =?us-ascii?Q?TB/DMs4xR7bqqRknXKxdjEH2xKlBPpJLk8aOwEacZgO+HLf1wTemeUf1kMTz?=
 =?us-ascii?Q?xae0mriK31hthJmYRJNvOW+r5Eg7PYlrQEWuDN/fIHWuYgD4HTtSPMCVRs0j?=
 =?us-ascii?Q?WtJ0b3bP76O9KPIfPuXa+rUAHvSb49tL9I1jc+QCofvrmw0F4CguWhhFdQfZ?=
 =?us-ascii?Q?Zw=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 94677eb5-afdd-493e-ec35-08dc75ed816b
X-MS-Exchange-CrossTenant-AuthSource: CY5PR11MB6366.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 May 2024 21:16:51.9767
 (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: 5j9NGIp1x+zV0imTW8qiPPkBKtTdSGzRLil4Y5NkY8XUyKKLx1z6WlHyhu9sbs2UF7t/EYFu53sSXx+RxFd/QWecz4AP+npT+59aiexyV8E=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB8739
X-OriginatorOrg: intel.com

On Wed, May 08, 2024 at 04:39:51PM +0800, Herbert Xu wrote:
> On Fri, Feb 09, 2024 at 01:43:42PM +0100, Damian Muszynski wrote:
> >
> > @@ -146,11 +147,19 @@ static void adf_device_reset_worker(struct work_struct *work)
> >  	adf_dev_restarted_notify(accel_dev);
> >  	clear_bit(ADF_STATUS_RESTARTING, &accel_dev->status);
> >  
> > -	/* The dev is back alive. Notify the caller if in sync mode */
> > -	if (reset_data->mode == ADF_DEV_RESET_SYNC)
> > -		complete(&reset_data->compl);
> > -	else
> > +	/*
> > +	 * The dev is back alive. Notify the caller if in sync mode
> > +	 *
> > +	 * If device restart will take a more time than expected,
> > +	 * the schedule_reset() function can timeout and exit. This can be
> > +	 * detected by calling the completion_done() function. In this case
> > +	 * the reset_data structure needs to be freed here.
> > +	 */
> > +	if (reset_data->mode == ADF_DEV_RESET_ASYNC ||
> > +	    completion_done(&reset_data->compl))
> >  		kfree(reset_data);
> > +	else
> > +		complete(&reset_data->compl);
> 
> This doesn't work because until you call complete, completion_done
> will always return false.  IOW we now have a memory leak instead of
> a UAF.
> 
> ---8<---
> Using completion_done to determine whether the caller has gone
> away only works after a complete call.  Furthermore it's still
> possible that the caller has not yet called wait_for_completion,
> resulting in another potential UAF.
> 
> Fix this by making the caller use cancel_work_sync and then freeing
> the memory safely.
> 
> Fixes: 7d42e097607c ("crypto: qat - resolve race condition during AER recovery")
> Cc: <stable@vger.kernel.org> #6.8+
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Reviewed-by: Giovanni Cabiddu <giovanni.cabiddu@intel.com>

This is also present in 6.6+ and 6.7+.

-- 
Giovanni