[PATCH 1/2] drm/radeon: Using unsigned long instead of unsigned to fix possible overflow

[PATCH 1/2] drm/radeon: Using unsigned long instead of unsigned to fix possible overflow

[Hanjun Guo]

VBIOSImageOffset in struct UEFI_ACPI_VFCT is ULONG (unsigned long),
but it will be assigned to "unsigned offset", so use unsigned long
instead of unsigned for the offset, to avoid possible overflow.

Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
---
 drivers/gpu/drm/radeon/radeon_bios.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/radeon_bios.c b/drivers/gpu/drm/radeon/radeon_bios.c
index 3312165..520d1d6 100644
--- a/drivers/gpu/drm/radeon/radeon_bios.c
+++ b/drivers/gpu/drm/radeon/radeon_bios.c
@@ -611,7 +611,7 @@ static bool radeon_acpi_vfct_bios(struct radeon_device *rdev)
 	struct acpi_table_header *hdr;
 	acpi_size tbl_size;
 	UEFI_ACPI_VFCT *vfct;
-	unsigned offset;
+	unsigned long offset;
 
 	if (!ACPI_SUCCESS(acpi_get_table("VFCT", 1, &hdr)))
 		return false;
-- 
1.7.12.4

[PATCH 2/2] drm/radeon: Add the missed acpi_put_table() to fix memory leak

[Hanjun Guo]

When the radeon driver reads the bios information from ACPI
table in radeon_acpi_vfct_bios(), it misses to call acpi_put_table()
to release the ACPI memory after the init, so add acpi_put_table()
properly to fix the memory leak.

Fixes: 268ba0a99f89 ("drm/radeon: implement ACPI VFCT vbios fetch (v3)")
Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
---
 drivers/gpu/drm/radeon/radeon_bios.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_bios.c b/drivers/gpu/drm/radeon/radeon_bios.c
index 520d1d6..16730c1 100644
--- a/drivers/gpu/drm/radeon/radeon_bios.c
+++ b/drivers/gpu/drm/radeon/radeon_bios.c
@@ -612,13 +612,14 @@ static bool radeon_acpi_vfct_bios(struct radeon_device *rdev)
 	acpi_size tbl_size;
 	UEFI_ACPI_VFCT *vfct;
 	unsigned long offset;
+	bool r = false;
 
 	if (!ACPI_SUCCESS(acpi_get_table("VFCT", 1, &hdr)))
 		return false;
 	tbl_size = hdr->length;
 	if (tbl_size < sizeof(UEFI_ACPI_VFCT)) {
 		DRM_ERROR("ACPI VFCT table present but broken (too short #1)\n");
-		return false;
+		goto out;
 	}
 
 	vfct = (UEFI_ACPI_VFCT *)hdr;
@@ -631,13 +632,13 @@ static bool radeon_acpi_vfct_bios(struct radeon_device *rdev)
 		offset += sizeof(VFCT_IMAGE_HEADER);
 		if (offset > tbl_size) {
 			DRM_ERROR("ACPI VFCT image header truncated\n");
-			return false;
+			goto out;
 		}
 
 		offset += vhdr->ImageLength;
 		if (offset > tbl_size) {
 			DRM_ERROR("ACPI VFCT image truncated\n");
-			return false;
+			goto out;
 		}
 
 		if (vhdr->ImageLength &&
@@ -649,15 +650,18 @@ static bool radeon_acpi_vfct_bios(struct radeon_device *rdev)
 			rdev->bios = kmemdup(&vbios->VbiosContent,
 					     vhdr->ImageLength,
 					     GFP_KERNEL);
+			if (rdev->bios)
+			   r = true;
 
-			if (!rdev->bios)
-				return false;
-			return true;
+			goto out;
 		}
 	}
 
 	DRM_ERROR("ACPI VFCT table present but broken (too short #2)\n");
-	return false;
+
+out:
+	acpi_put_table(hdr);
+	return r;
 }
 #else
 static inline bool radeon_acpi_vfct_bios(struct radeon_device *rdev)
-- 
1.7.12.4

Re: [PATCH 1/2] drm/radeon: Using unsigned long instead of unsigned to fix possible overflow

[Alex Deucher]

On Fri, Nov 4, 2022 at 6:05 AM Hanjun Guo <guohanjun@huawei.com> wrote:
>
> VBIOSImageOffset in struct UEFI_ACPI_VFCT is ULONG (unsigned long),
> but it will be assigned to "unsigned offset", so use unsigned long
> instead of unsigned for the offset, to avoid possible overflow.

ULONG in atombios is 32 bits so an int should be fine.

Alex

>
> Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
> ---
>  drivers/gpu/drm/radeon/radeon_bios.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_bios.c b/drivers/gpu/drm/radeon/radeon_bios.c
> index 3312165..520d1d6 100644
> --- a/drivers/gpu/drm/radeon/radeon_bios.c
> +++ b/drivers/gpu/drm/radeon/radeon_bios.c
> @@ -611,7 +611,7 @@ static bool radeon_acpi_vfct_bios(struct radeon_device *rdev)
>         struct acpi_table_header *hdr;
>         acpi_size tbl_size;
>         UEFI_ACPI_VFCT *vfct;
> -       unsigned offset;
> +       unsigned long offset;
>
>         if (!ACPI_SUCCESS(acpi_get_table("VFCT", 1, &hdr)))
>                 return false;
> --
> 1.7.12.4
>

Re: [PATCH 1/2] drm/radeon: Using unsigned long instead of unsigned to fix possible overflow

[Hanjun Guo]

On 2022/11/5 2:31, Alex Deucher wrote:
> On Fri, Nov 4, 2022 at 6:05 AM Hanjun Guo <guohanjun@huawei.com> wrote:
>>
>> VBIOSImageOffset in struct UEFI_ACPI_VFCT is ULONG (unsigned long),
>> but it will be assigned to "unsigned offset", so use unsigned long
>> instead of unsigned for the offset, to avoid possible overflow.
> 
> ULONG in atombios is 32 bits so an int should be fine.

I saw the typedef in the atombios.h is unsigned long but I missed
that the real one is uint32_t in atom-types.h, please drop this patch
and take a look at the PATCH 2/2.

Thanks for the prompt reply.

Thanks
Hanjun

Re: [PATCH 2/2] drm/radeon: Add the missed acpi_put_table() to fix memory leak

[Alex Deucher]

Applied.  Thanks

On Fri, Nov 4, 2022 at 6:05 AM Hanjun Guo <guohanjun@huawei.com> wrote:
>
> When the radeon driver reads the bios information from ACPI
> table in radeon_acpi_vfct_bios(), it misses to call acpi_put_table()
> to release the ACPI memory after the init, so add acpi_put_table()
> properly to fix the memory leak.
>
> Fixes: 268ba0a99f89 ("drm/radeon: implement ACPI VFCT vbios fetch (v3)")
> Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
> ---
>  drivers/gpu/drm/radeon/radeon_bios.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_bios.c b/drivers/gpu/drm/radeon/radeon_bios.c
> index 520d1d6..16730c1 100644
> --- a/drivers/gpu/drm/radeon/radeon_bios.c
> +++ b/drivers/gpu/drm/radeon/radeon_bios.c
> @@ -612,13 +612,14 @@ static bool radeon_acpi_vfct_bios(struct radeon_device *rdev)
>         acpi_size tbl_size;
>         UEFI_ACPI_VFCT *vfct;
>         unsigned long offset;
> +       bool r = false;
>
>         if (!ACPI_SUCCESS(acpi_get_table("VFCT", 1, &hdr)))
>                 return false;
>         tbl_size = hdr->length;
>         if (tbl_size < sizeof(UEFI_ACPI_VFCT)) {
>                 DRM_ERROR("ACPI VFCT table present but broken (too short #1)\n");
> -               return false;
> +               goto out;
>         }
>
>         vfct = (UEFI_ACPI_VFCT *)hdr;
> @@ -631,13 +632,13 @@ static bool radeon_acpi_vfct_bios(struct radeon_device *rdev)
>                 offset += sizeof(VFCT_IMAGE_HEADER);
>                 if (offset > tbl_size) {
>                         DRM_ERROR("ACPI VFCT image header truncated\n");
> -                       return false;
> +                       goto out;
>                 }
>
>                 offset += vhdr->ImageLength;
>                 if (offset > tbl_size) {
>                         DRM_ERROR("ACPI VFCT image truncated\n");
> -                       return false;
> +                       goto out;
>                 }
>
>                 if (vhdr->ImageLength &&
> @@ -649,15 +650,18 @@ static bool radeon_acpi_vfct_bios(struct radeon_device *rdev)
>                         rdev->bios = kmemdup(&vbios->VbiosContent,
>                                              vhdr->ImageLength,
>                                              GFP_KERNEL);
> +                       if (rdev->bios)
> +                          r = true;
>
> -                       if (!rdev->bios)
> -                               return false;
> -                       return true;
> +                       goto out;
>                 }
>         }
>
>         DRM_ERROR("ACPI VFCT table present but broken (too short #2)\n");
> -       return false;
> +
> +out:
> +       acpi_put_table(hdr);
> +       return r;
>  }
>  #else
>  static inline bool radeon_acpi_vfct_bios(struct radeon_device *rdev)
> --
> 1.7.12.4
>