linux-kernel - Re: [PATCH] drm/amdkfd: Put ACPI table after using it

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <443ace32-0860-f823-bc3f-3faafd5da54e@amd.com>
Date:   Thu, 30 Jul 2020 22:41:46 -0400
From:   Felix Kuehling <felix.kuehling@....com>
To:     Hanjun Guo <guohanjun@...wei.com>,
        Alex Deucher <alexander.deucher@....com>,
        Christian König <christian.koenig@....com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/amdkfd: Put ACPI table after using it

Hi Hanjun,

Sorry for the late reply.

Thank you for the patch and the explanation. This seems to have been
broken since the first version of KFD in 2014. See one suggestion inline.

Am 2020-07-22 um 5:48 a.m. schrieb Hanjun Guo:
> The acpi_get_table() should be coupled with acpi_put_table() if
> the mapped table is not used at runtime to release the table
> mapping.
>
> In kfd_create_crat_image_acpi(), crat_table is copied to pcrat_image,
> and in kfd_create_vcrat_image_cpu(), the acpi_table is only used to
> get the OEM info, so those table mappings need to be release after
> using it.
>
> Signed-off-by: Hanjun Guo <guohanjun@...wei.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_crat.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> index 1009a3b..d378e61 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_crat.c
> @@ -756,6 +756,7 @@ int kfd_create_crat_image_acpi(void **crat_image, size_t *size)
>  	struct acpi_table_header *crat_table;
>  	acpi_status status;
>  	void *pcrat_image;
> +	int rc = 0;
>  
>  	if (!crat_image)
>  		return -EINVAL;
> @@ -776,17 +777,21 @@ int kfd_create_crat_image_acpi(void **crat_image, size_t *size)
>  
>  	if (ignore_crat) {
>  		pr_info("CRAT table disabled by module option\n");

We should probably move this check to before we get the CRAT table.
There is not point getting and putting it if we're going to ignore it
anyway.

Do you want to send an updated patch with that change? Or maybe do it as
a 2-patch series?

Regards,
  Felix


> -		return -ENODATA;
> +		rc = -ENODATA;
> +		goto out;
>  	}
>  
>  	pcrat_image = kmemdup(crat_table, crat_table->length, GFP_KERNEL);
> -	if (!pcrat_image)
> -		return -ENOMEM;
> +	if (!pcrat_image) {
> +		rc = -ENOMEM;
> +		goto out;
> +	}
>  
>  	*crat_image = pcrat_image;
>  	*size = crat_table->length;
> -
> -	return 0;
> +out:
> +	acpi_put_table(crat_table);
> +	return rc;
>  }
>  
>  /* Memory required to create Virtual CRAT.
> @@ -970,6 +975,7 @@ static int kfd_create_vcrat_image_cpu(void *pcrat_image, size_t *size)
>  				CRAT_OEMID_LENGTH);
>  		memcpy(crat_table->oem_table_id, acpi_table->oem_table_id,
>  				CRAT_OEMTABLEID_LENGTH);
> +		acpi_put_table(acpi_table);
>  	}
>  	crat_table->total_entries = 0;
>  	crat_table->num_domains = 0;