AMD-GFX Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/amdgpu: Fix pointer casts when reading dynamic region sizes
@ 2025-10-22 13:34 Srinivasan Shanmugam
  2025-10-22 13:52 ` Lazar, Lijo
  2025-10-23  5:35 ` [PATCH v2] " Srinivasan Shanmugam
  0 siblings, 2 replies; 5+ messages in thread
From: Srinivasan Shanmugam @ 2025-10-22 13:34 UTC (permalink / raw)
  To: Christian König, Alex Deucher
  Cc: amd-gfx, Srinivasan Shanmugam, Ellen Pan

The function amdgpu_virt_get_dynamic_data_info() writes a 64-bit size
value.  In two places (amdgpu_bios.c and amdgpu_discovery.c), the code
passed the address of a smaller variable by casting it to u64 *, which
is unsafe.

This could make the function write more bytes than the smaller variable
can hold, possibly overwriting nearby memory.  Reported by static
analysis tools.

Fix it by using a local u64 variable (tmp_size) to store the size, then
assign it to the smaller destination field.

Fixes: ae92010fb321 ("drm/amdgpu: Add logic for VF ipd and VF bios to init from dynamic crit_region offsets")
Reported by: Dan Carpenter <dan.carpenter@linaro.org>
Cc: Ellen Pan <yunru.pan@amd.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c      | 4 +++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
index db705bf723f1..eb7ba7c593bf 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
@@ -104,6 +104,7 @@ static bool amdgpu_read_bios_from_vram(struct amdgpu_device *adev)
 	uint8_t __iomem *bios = NULL;
 	resource_size_t vram_base;
 	resource_size_t size = 256 * 1024; /* ??? */
+	u64 tmp_size = 0;
 
 	if (!(adev->flags & AMD_IS_APU))
 		if (amdgpu_device_need_post(adev))
@@ -126,10 +127,11 @@ static bool amdgpu_read_bios_from_vram(struct amdgpu_device *adev)
 	 */
 	if (amdgpu_sriov_vf(adev) && adev->virt.is_dynamic_crit_regn_enabled) {
 		if (amdgpu_virt_get_dynamic_data_info(adev,
-				AMD_SRIOV_MSG_VBIOS_IMG_TABLE_ID, adev->bios, (uint64_t *)&size)) {
+				AMD_SRIOV_MSG_VBIOS_IMG_TABLE_ID, adev->bios, &tmp_size)) {
 			amdgpu_bios_release(adev);
 			return false;
 		}
+		adev->bios_size = (resource_size_t)tmp_size;
 	} else {
 		bios = ioremap_wc(vram_base, size);
 		if (!bios) {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
index a7cb4665f485..87f024f72a59 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
@@ -275,6 +275,7 @@ static int amdgpu_discovery_read_binary_from_mem(struct amdgpu_device *adev,
 	uint64_t vram_size;
 	int i, ret = 0;
 	u32 msg;
+	u64 tmp_size = 0;
 
 	if (!amdgpu_sriov_vf(adev)) {
 		/* It can take up to two second for IFWI init to complete on some dGPUs,
@@ -311,12 +312,13 @@ static int amdgpu_discovery_read_binary_from_mem(struct amdgpu_device *adev,
 			 */
 			if (amdgpu_virt_get_dynamic_data_info(adev,
 						AMD_SRIOV_MSG_IPD_TABLE_ID, binary,
-						(uint64_t *)&adev->discovery.size)) {
+						&tmp_size)) {
 				dev_err(adev->dev,
 						"failed to read discovery info from dynamic critical region.");
 				ret = -EINVAL;
 				goto exit;
 			}
+			adev->discovery.size = (u32)tmp_size;
 		} else {
 			uint64_t pos = vram_size - DISCOVERY_TMR_OFFSET;
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] drm/amdgpu: Fix pointer casts when reading dynamic region sizes
  2025-10-22 13:34 [PATCH] drm/amdgpu: Fix pointer casts when reading dynamic region sizes Srinivasan Shanmugam
@ 2025-10-22 13:52 ` Lazar, Lijo
  2025-10-22 14:46   ` Pan, Ellen
  2025-10-23  5:35 ` [PATCH v2] " Srinivasan Shanmugam
  1 sibling, 1 reply; 5+ messages in thread
From: Lazar, Lijo @ 2025-10-22 13:52 UTC (permalink / raw)
  To: SHANMUGAM, SRINIVASAN, Koenig, Christian, Deucher, Alexander
  Cc: amd-gfx@lists.freedesktop.org, SHANMUGAM, SRINIVASAN, Pan, Ellen

[-- Attachment #1: Type: text/plain, Size: 4526 bytes --]

[Public]

You may change the function signature to u32 *. I don't think any table of larger size is expected. Ellen, please confirm.

Thanks,
Lijo
________________________________
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> on behalf of Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
Sent: Wednesday, October 22, 2025 7:04:25 PM
To: Koenig, Christian <Christian.Koenig@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>
Cc: amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>; SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM@amd.com>; Pan, Ellen <Yunru.Pan@amd.com>
Subject: [PATCH] drm/amdgpu: Fix pointer casts when reading dynamic region sizes

The function amdgpu_virt_get_dynamic_data_info() writes a 64-bit size
value.  In two places (amdgpu_bios.c and amdgpu_discovery.c), the code
passed the address of a smaller variable by casting it to u64 *, which
is unsafe.

This could make the function write more bytes than the smaller variable
can hold, possibly overwriting nearby memory.  Reported by static
analysis tools.

Fix it by using a local u64 variable (tmp_size) to store the size, then
assign it to the smaller destination field.

Fixes: ae92010fb321 ("drm/amdgpu: Add logic for VF ipd and VF bios to init from dynamic crit_region offsets")
Reported by: Dan Carpenter <dan.carpenter@linaro.org>
Cc: Ellen Pan <yunru.pan@amd.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c      | 4 +++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
index db705bf723f1..eb7ba7c593bf 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
@@ -104,6 +104,7 @@ static bool amdgpu_read_bios_from_vram(struct amdgpu_device *adev)
         uint8_t __iomem *bios = NULL;
         resource_size_t vram_base;
         resource_size_t size = 256 * 1024; /* ??? */
+       u64 tmp_size = 0;

         if (!(adev->flags & AMD_IS_APU))
                 if (amdgpu_device_need_post(adev))
@@ -126,10 +127,11 @@ static bool amdgpu_read_bios_from_vram(struct amdgpu_device *adev)
          */
         if (amdgpu_sriov_vf(adev) && adev->virt.is_dynamic_crit_regn_enabled) {
                 if (amdgpu_virt_get_dynamic_data_info(adev,
-                               AMD_SRIOV_MSG_VBIOS_IMG_TABLE_ID, adev->bios, (uint64_t *)&size)) {
+                               AMD_SRIOV_MSG_VBIOS_IMG_TABLE_ID, adev->bios, &tmp_size)) {
                         amdgpu_bios_release(adev);
                         return false;
                 }
+               adev->bios_size = (resource_size_t)tmp_size;
         } else {
                 bios = ioremap_wc(vram_base, size);
                 if (!bios) {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
index a7cb4665f485..87f024f72a59 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
@@ -275,6 +275,7 @@ static int amdgpu_discovery_read_binary_from_mem(struct amdgpu_device *adev,
         uint64_t vram_size;
         int i, ret = 0;
         u32 msg;
+       u64 tmp_size = 0;

         if (!amdgpu_sriov_vf(adev)) {
                 /* It can take up to two second for IFWI init to complete on some dGPUs,
@@ -311,12 +312,13 @@ static int amdgpu_discovery_read_binary_from_mem(struct amdgpu_device *adev,
                          */
                         if (amdgpu_virt_get_dynamic_data_info(adev,
                                                 AMD_SRIOV_MSG_IPD_TABLE_ID, binary,
-                                               (uint64_t *)&adev->discovery.size)) {
+                                               &tmp_size)) {
                                 dev_err(adev->dev,
                                                 "failed to read discovery info from dynamic critical region.");
                                 ret = -EINVAL;
                                 goto exit;
                         }
+                       adev->discovery.size = (u32)tmp_size;
                 } else {
                         uint64_t pos = vram_size - DISCOVERY_TMR_OFFSET;

--
2.34.1


[-- Attachment #2: Type: text/html, Size: 10359 bytes --]

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* RE: [PATCH] drm/amdgpu: Fix pointer casts when reading dynamic region sizes
  2025-10-22 13:52 ` Lazar, Lijo
@ 2025-10-22 14:46   ` Pan, Ellen
  0 siblings, 0 replies; 5+ messages in thread
From: Pan, Ellen @ 2025-10-22 14:46 UTC (permalink / raw)
  To: Lazar, Lijo, SHANMUGAM, SRINIVASAN, Koenig, Christian,
	Deucher, Alexander
  Cc: amd-gfx@lists.freedesktop.org, SHANMUGAM, SRINIVASAN

[-- Attachment #1: Type: text/plain, Size: 5596 bytes --]

[Public]

Hi Lijo and Sri,

Confirmed that no larger size than u32 is expected for dynamic tables.
I'll update the patch and send out the fix soon.

Thanks,
Ellen

From: Lazar, Lijo <Lijo.Lazar@amd.com>
Sent: Wednesday, October 22, 2025 9:52 AM
To: SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>
Cc: amd-gfx@lists.freedesktop.org; SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM@amd.com>; Pan, Ellen <Yunru.Pan@amd.com>
Subject: Re: [PATCH] drm/amdgpu: Fix pointer casts when reading dynamic region sizes


[Public]

You may change the function signature to u32 *. I don't think any table of larger size is expected. Ellen, please confirm.

Thanks,
Lijo
________________________________
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org<mailto:amd-gfx-bounces@lists.freedesktop.org>> on behalf of Srinivasan Shanmugam <srinivasan.shanmugam@amd.com<mailto:srinivasan.shanmugam@amd.com>>
Sent: Wednesday, October 22, 2025 7:04:25 PM
To: Koenig, Christian <Christian.Koenig@amd.com<mailto:Christian.Koenig@amd.com>>; Deucher, Alexander <Alexander.Deucher@amd.com<mailto:Alexander.Deucher@amd.com>>
Cc: amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org> <amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>>; SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM@amd.com<mailto:SRINIVASAN.SHANMUGAM@amd.com>>; Pan, Ellen <Yunru.Pan@amd.com<mailto:Yunru.Pan@amd.com>>
Subject: [PATCH] drm/amdgpu: Fix pointer casts when reading dynamic region sizes

The function amdgpu_virt_get_dynamic_data_info() writes a 64-bit size
value.  In two places (amdgpu_bios.c and amdgpu_discovery.c), the code
passed the address of a smaller variable by casting it to u64 *, which
is unsafe.

This could make the function write more bytes than the smaller variable
can hold, possibly overwriting nearby memory.  Reported by static
analysis tools.

Fix it by using a local u64 variable (tmp_size) to store the size, then
assign it to the smaller destination field.

Fixes: ae92010fb321 ("drm/amdgpu: Add logic for VF ipd and VF bios to init from dynamic crit_region offsets")
Reported by: Dan Carpenter <dan.carpenter@linaro.org<mailto:dan.carpenter@linaro.org>>
Cc: Ellen Pan <yunru.pan@amd.com<mailto:yunru.pan@amd.com>>
Cc: Christian König <christian.koenig@amd.com<mailto:christian.koenig@amd.com>>
Cc: Alex Deucher <alexander.deucher@amd.com<mailto:alexander.deucher@amd.com>>
Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com<mailto:srinivasan.shanmugam@amd.com>>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c      | 4 +++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
index db705bf723f1..eb7ba7c593bf 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
@@ -104,6 +104,7 @@ static bool amdgpu_read_bios_from_vram(struct amdgpu_device *adev)
         uint8_t __iomem *bios = NULL;
         resource_size_t vram_base;
         resource_size_t size = 256 * 1024; /* ??? */
+       u64 tmp_size = 0;

         if (!(adev->flags & AMD_IS_APU))
                 if (amdgpu_device_need_post(adev))
@@ -126,10 +127,11 @@ static bool amdgpu_read_bios_from_vram(struct amdgpu_device *adev)
          */
         if (amdgpu_sriov_vf(adev) && adev->virt.is_dynamic_crit_regn_enabled) {
                 if (amdgpu_virt_get_dynamic_data_info(adev,
-                               AMD_SRIOV_MSG_VBIOS_IMG_TABLE_ID, adev->bios, (uint64_t *)&size)) {
+                               AMD_SRIOV_MSG_VBIOS_IMG_TABLE_ID, adev->bios, &tmp_size)) {
                         amdgpu_bios_release(adev);
                         return false;
                 }
+               adev->bios_size = (resource_size_t)tmp_size;
         } else {
                 bios = ioremap_wc(vram_base, size);
                 if (!bios) {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
index a7cb4665f485..87f024f72a59 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
@@ -275,6 +275,7 @@ static int amdgpu_discovery_read_binary_from_mem(struct amdgpu_device *adev,
         uint64_t vram_size;
         int i, ret = 0;
         u32 msg;
+       u64 tmp_size = 0;

         if (!amdgpu_sriov_vf(adev)) {
                 /* It can take up to two second for IFWI init to complete on some dGPUs,
@@ -311,12 +312,13 @@ static int amdgpu_discovery_read_binary_from_mem(struct amdgpu_device *adev,
                          */
                         if (amdgpu_virt_get_dynamic_data_info(adev,
                                                 AMD_SRIOV_MSG_IPD_TABLE_ID, binary,
-                                               (uint64_t *)&adev->discovery.size)) {
+                                               &tmp_size)) {
                                 dev_err(adev->dev,
                                                 "failed to read discovery info from dynamic critical region.");
                                 ret = -EINVAL;
                                 goto exit;
                         }
+                       adev->discovery.size = (u32)tmp_size;
                 } else {
                         uint64_t pos = vram_size - DISCOVERY_TMR_OFFSET;

--
2.34.1

[-- Attachment #2: Type: text/html, Size: 14162 bytes --]

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH v2] drm/amdgpu: Fix pointer casts when reading dynamic region sizes
  2025-10-22 13:34 [PATCH] drm/amdgpu: Fix pointer casts when reading dynamic region sizes Srinivasan Shanmugam
  2025-10-22 13:52 ` Lazar, Lijo
@ 2025-10-23  5:35 ` Srinivasan Shanmugam
  2025-10-23  6:23   ` Lazar, Lijo
  1 sibling, 1 reply; 5+ messages in thread
From: Srinivasan Shanmugam @ 2025-10-23  5:35 UTC (permalink / raw)
  To: Christian König, Alex Deucher
  Cc: amd-gfx, Srinivasan Shanmugam, Ellen Pan

The function amdgpu_virt_get_dynamic_data_info() writes a 64-bit size
value.  In two places (amdgpu_bios.c and amdgpu_discovery.c), the code
passed the address of a smaller variable by casting it to u64 *, which
is unsafe.

This could make the function write more bytes than the smaller variable
can hold, possibly overwriting nearby memory. Reported by static
analysis tools.

v2: Dynamic region size comes from the host (SR-IOV setup) and is always
fixed to 5 MB. (Lijo/Ellen)

5 MB easily fits inside a 32-bit value, so using a 64-bit type is not
needed. It also avoids extra type casts

Fixes: ae92010fb321 ("drm/amdgpu: Add logic for VF ipd and VF bios to init from dynamic crit_region offsets")
Reported by: Dan Carpenter <dan.carpenter@linaro.org>
Cc: Ellen Pan <yunru.pan@amd.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c      | 4 ++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c      | 2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h      | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
index db705bf723f1..35d04e69aec0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
@@ -103,7 +103,7 @@ static bool amdgpu_read_bios_from_vram(struct amdgpu_device *adev)
 {
 	uint8_t __iomem *bios = NULL;
 	resource_size_t vram_base;
-	resource_size_t size = 256 * 1024; /* ??? */
+	u32 size = 256U * 1024U; /* ??? */
 
 	if (!(adev->flags & AMD_IS_APU))
 		if (amdgpu_device_need_post(adev))
@@ -126,7 +126,7 @@ static bool amdgpu_read_bios_from_vram(struct amdgpu_device *adev)
 	 */
 	if (amdgpu_sriov_vf(adev) && adev->virt.is_dynamic_crit_regn_enabled) {
 		if (amdgpu_virt_get_dynamic_data_info(adev,
-				AMD_SRIOV_MSG_VBIOS_IMG_TABLE_ID, adev->bios, (uint64_t *)&size)) {
+				AMD_SRIOV_MSG_VBIOS_IMG_TABLE_ID, adev->bios, &size)) {
 			amdgpu_bios_release(adev);
 			return false;
 		}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
index a7cb4665f485..fa2a22dfa048 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
@@ -311,7 +311,7 @@ static int amdgpu_discovery_read_binary_from_mem(struct amdgpu_device *adev,
 			 */
 			if (amdgpu_virt_get_dynamic_data_info(adev,
 						AMD_SRIOV_MSG_IPD_TABLE_ID, binary,
-						(uint64_t *)&adev->discovery.size)) {
+						&adev->discovery.size)) {
 				dev_err(adev->dev,
 						"failed to read discovery info from dynamic critical region.");
 				ret = -EINVAL;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
index 66e9cd103597..38cc446500d3 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
@@ -1101,7 +1101,7 @@ int amdgpu_virt_init_critical_region(struct amdgpu_device *adev)
 }
 
 int amdgpu_virt_get_dynamic_data_info(struct amdgpu_device *adev,
-	int data_id, uint8_t *binary, uint64_t *size)
+	int data_id, uint8_t *binary, u32 *size)
 {
 	uint32_t data_offset = 0;
 	uint32_t data_size = 0;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
index 2a13cc892a13..14d864be5800 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
@@ -443,7 +443,7 @@ void amdgpu_virt_init(struct amdgpu_device *adev);
 
 int amdgpu_virt_init_critical_region(struct amdgpu_device *adev);
 int amdgpu_virt_get_dynamic_data_info(struct amdgpu_device *adev,
-	int data_id, uint8_t *binary, uint64_t *size);
+	int data_id, uint8_t *binary, u32 *size);
 
 bool amdgpu_virt_can_access_debugfs(struct amdgpu_device *adev);
 int amdgpu_virt_enable_access_debugfs(struct amdgpu_device *adev);
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v2] drm/amdgpu: Fix pointer casts when reading dynamic region sizes
  2025-10-23  5:35 ` [PATCH v2] " Srinivasan Shanmugam
@ 2025-10-23  6:23   ` Lazar, Lijo
  0 siblings, 0 replies; 5+ messages in thread
From: Lazar, Lijo @ 2025-10-23  6:23 UTC (permalink / raw)
  To: Srinivasan Shanmugam, Christian König, Alex Deucher
  Cc: amd-gfx, Ellen Pan



On 10/23/2025 11:05 AM, Srinivasan Shanmugam wrote:
> The function amdgpu_virt_get_dynamic_data_info() writes a 64-bit size
> value.  In two places (amdgpu_bios.c and amdgpu_discovery.c), the code
> passed the address of a smaller variable by casting it to u64 *, which
> is unsafe.
> 
> This could make the function write more bytes than the smaller variable
> can hold, possibly overwriting nearby memory. Reported by static
> analysis tools.
> 
> v2: Dynamic region size comes from the host (SR-IOV setup) and is always
> fixed to 5 MB. (Lijo/Ellen)
> 
> 5 MB easily fits inside a 32-bit value, so using a 64-bit type is not
> needed. It also avoids extra type casts
> 
> Fixes: ae92010fb321 ("drm/amdgpu: Add logic for VF ipd and VF bios to init from dynamic crit_region offsets")
> Reported by: Dan Carpenter <dan.carpenter@linaro.org>
> Cc: Ellen Pan <yunru.pan@amd.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>

Reviewed-by: Lijo Lazar <lijo.lazar@amd.com>

Thanks,
Lijo

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c      | 4 ++--
>   drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c | 2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c      | 2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h      | 2 +-
>   4 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> index db705bf723f1..35d04e69aec0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> @@ -103,7 +103,7 @@ static bool amdgpu_read_bios_from_vram(struct amdgpu_device *adev)
>   {
>   	uint8_t __iomem *bios = NULL;
>   	resource_size_t vram_base;
> -	resource_size_t size = 256 * 1024; /* ??? */
> +	u32 size = 256U * 1024U; /* ??? */
>   
>   	if (!(adev->flags & AMD_IS_APU))
>   		if (amdgpu_device_need_post(adev))
> @@ -126,7 +126,7 @@ static bool amdgpu_read_bios_from_vram(struct amdgpu_device *adev)
>   	 */
>   	if (amdgpu_sriov_vf(adev) && adev->virt.is_dynamic_crit_regn_enabled) {
>   		if (amdgpu_virt_get_dynamic_data_info(adev,
> -				AMD_SRIOV_MSG_VBIOS_IMG_TABLE_ID, adev->bios, (uint64_t *)&size)) {
> +				AMD_SRIOV_MSG_VBIOS_IMG_TABLE_ID, adev->bios, &size)) {
>   			amdgpu_bios_release(adev);
>   			return false;
>   		}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> index a7cb4665f485..fa2a22dfa048 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> @@ -311,7 +311,7 @@ static int amdgpu_discovery_read_binary_from_mem(struct amdgpu_device *adev,
>   			 */
>   			if (amdgpu_virt_get_dynamic_data_info(adev,
>   						AMD_SRIOV_MSG_IPD_TABLE_ID, binary,
> -						(uint64_t *)&adev->discovery.size)) {
> +						&adev->discovery.size)) {
>   				dev_err(adev->dev,
>   						"failed to read discovery info from dynamic critical region.");
>   				ret = -EINVAL;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> index 66e9cd103597..38cc446500d3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> @@ -1101,7 +1101,7 @@ int amdgpu_virt_init_critical_region(struct amdgpu_device *adev)
>   }
>   
>   int amdgpu_virt_get_dynamic_data_info(struct amdgpu_device *adev,
> -	int data_id, uint8_t *binary, uint64_t *size)
> +	int data_id, uint8_t *binary, u32 *size)
>   {
>   	uint32_t data_offset = 0;
>   	uint32_t data_size = 0;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
> index 2a13cc892a13..14d864be5800 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.h
> @@ -443,7 +443,7 @@ void amdgpu_virt_init(struct amdgpu_device *adev);
>   
>   int amdgpu_virt_init_critical_region(struct amdgpu_device *adev);
>   int amdgpu_virt_get_dynamic_data_info(struct amdgpu_device *adev,
> -	int data_id, uint8_t *binary, uint64_t *size);
> +	int data_id, uint8_t *binary, u32 *size);
>   
>   bool amdgpu_virt_can_access_debugfs(struct amdgpu_device *adev);
>   int amdgpu_virt_enable_access_debugfs(struct amdgpu_device *adev);


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2025-10-23  6:23 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-22 13:34 [PATCH] drm/amdgpu: Fix pointer casts when reading dynamic region sizes Srinivasan Shanmugam
2025-10-22 13:52 ` Lazar, Lijo
2025-10-22 14:46   ` Pan, Ellen
2025-10-23  5:35 ` [PATCH v2] " Srinivasan Shanmugam
2025-10-23  6:23   ` Lazar, Lijo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox