linux-kernel - Re: [PATCH] drm/xe/query: Refactor copy_to

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 for Android: free password hash cracker in your pocket

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

Message-ID: <87plp1b989.fsf@intel.com>
Date: Wed, 18 Sep 2024 12:29:42 +0300
From: Jani Nikula <jani.nikula@...el.com>
To: Markus Elfring <Markus.Elfring@....de>, intel-xe@...ts.freedesktop.org,
 dri-devel@...ts.freedesktop.org, kernel-janitors@...r.kernel.org, Daniel
 Vetter <daniel.vetter@...ll.ch>, David Airlie <airlied@...il.com>, Faith
 Ekstrand <faith.ekstrand@...labora.com>, Francois Dugast
 <francois.dugast@...el.com>, José Roberto de Souza
 <jose.souza@...el.com>,
 Lucas De Marchi <lucas.demarchi@...el.com>, Maarten Lankhorst
 <maarten.lankhorst@...ux.intel.com>, Matt Roper
 <matthew.d.roper@...el.com>, Matthew Brost <matthew.brost@...el.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>, Maxime Ripard
 <mripard@...nel.org>, Nirmoy Das <nirmoy.das@...el.com>, Philippe Lecluse
 <philippe.lecluse@...el.com>, Rodrigo Vivi <rodrigo.vivi@...el.com>,
 Simona Vetter <simona@...ll.ch>, Thomas Hellström
 <thomas.hellstrom@...ux.intel.com>, Thomas Zimmermann
 <tzimmermann@...e.de>
Cc: LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] drm/xe/query: Refactor copy_to_user() usage in four
 functions

On Wed, 18 Sep 2024, Markus Elfring <Markus.Elfring@....de> wrote:
> From: Markus Elfring <elfring@...rs.sourceforge.net>
> Date: Wed, 18 Sep 2024 09:43:07 +0200
>
> Assign return values from copy_to_user() calls to additional local variables
> so that four kfree() calls and return statements can be omitted accordingly.
>
> This issue was transformed by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@...rs.sourceforge.net>
> ---
>  drivers/gpu/drm/xe/xe_query.c | 32 ++++++++++++--------------------
>  1 file changed, 12 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c
> index 5246a4a2740e..6195e720176d 100644
> --- a/drivers/gpu/drm/xe/xe_query.c
> +++ b/drivers/gpu/drm/xe/xe_query.c
> @@ -220,13 +220,11 @@ static int query_engines(struct xe_device *xe,
>
>  	engines->num_engines = i;
>
> -	if (copy_to_user(query_ptr, engines, size)) {
> +	{

Please don't leave blocks like this behind when you remove the if.

BR,
Jani.

> +		unsigned long ctu = copy_to_user(query_ptr, engines, size);
>  		kfree(engines);
> -		return -EFAULT;
> +		return ctu ? -EFAULT : 0;
>  	}
> -	kfree(engines);
> -
> -	return 0;
>  }
>
>  static size_t calc_mem_regions_size(struct xe_device *xe)
> @@ -344,13 +342,11 @@ static int query_config(struct xe_device *xe, struct drm_xe_device_query *query)
>  	config->info[DRM_XE_QUERY_CONFIG_MAX_EXEC_QUEUE_PRIORITY] =
>  		xe_exec_queue_device_get_max_priority(xe);
>
> -	if (copy_to_user(query_ptr, config, size)) {
> +	{
> +		unsigned long ctu = copy_to_user(query_ptr, config, size);
>  		kfree(config);
> -		return -EFAULT;
> +		return ctu ? -EFAULT : 0;
>  	}
> -	kfree(config);
> -
> -	return 0;
>  }
>
>  static int query_gt_list(struct xe_device *xe, struct drm_xe_device_query *query)
> @@ -414,13 +410,11 @@ static int query_gt_list(struct xe_device *xe, struct drm_xe_device_query *query
>  			REG_FIELD_GET(GMD_ID_REVID, gt->info.gmdid);
>  	}
>
> -	if (copy_to_user(query_ptr, gt_list, size)) {
> +	{
> +		unsigned long ctu = copy_to_user(query_ptr, gt_list, size);
>  		kfree(gt_list);
> -		return -EFAULT;
> +		return ctu ? -EFAULT : 0;
>  	}
> -	kfree(gt_list);
> -
> -	return 0;
>  }
>
>  static int query_hwconfig(struct xe_device *xe,
> @@ -444,13 +438,11 @@ static int query_hwconfig(struct xe_device *xe,
>
>  	xe_guc_hwconfig_copy(&gt->uc.guc, hwconfig);
>
> -	if (copy_to_user(query_ptr, hwconfig, size)) {
> +	{
> +		unsigned long ctu = copy_to_user(query_ptr, hwconfig, size);
>  		kfree(hwconfig);
> -		return -EFAULT;
> +		return ctu ? -EFAULT : 0;
>  	}
> -	kfree(hwconfig);
> -
> -	return 0;
>  }
>
>  static size_t calc_topo_query_size(struct xe_device *xe)
> --
> 2.46.0
>

-- 
Jani Nikula, Intel