RE: [PATCH] drm/amdgpu: refine amdgpu_fru_get_product_info

["Chen, Jiansong (Simon)" <Jiansong.Chen@amd.com>]

[AMD Official Use Only]

Please ignore the patch, will resend after removing multiple assignments.

-----Original Message-----
From: Jiansong Chen <Jiansong.Chen@amd.com>
Sent: Tuesday, May 25, 2021 2:17 PM
To: amd-gfx@lists.freedesktop.org
Cc: Chen, Jiansong (Simon) <Jiansong.Chen@amd.com>
Subject: [PATCH] drm/amdgpu: refine amdgpu_fru_get_product_info

1. eliminate potential array index out of bounds.
2. return meaningful value for failure.

Signed-off-by: Jiansong Chen <Jiansong.Chen@amd.com>
Change-Id: I9be36eb2e42ee46cd00464b0f2c35a4e4ea213e3
---
 .../gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c    | 69 ++++++++++---------
 1 file changed, 35 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
index 8f4a8f8d8146..5c2b4403a5b6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fru_eeprom.c
@@ -101,7 +101,8 @@ static int amdgpu_fru_read_eeprom(struct amdgpu_device *adev, uint32_t addrptr,  int amdgpu_fru_get_product_info(struct amdgpu_device *adev)  {
        unsigned char buff[34];
-       int addrptr = 0, size = 0;
+       int addrptr, size;
+       int len;

        if (!is_fru_eeprom_supported(adev))
                return 0;
@@ -109,7 +110,7 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
        /* If algo exists, it means that the i2c_adapter's initialized */
        if (!adev->pm.smu_i2c.algo) {
                DRM_WARN("Cannot access FRU, EEPROM accessor not initialized");
-               return 0;
+               return -ENODEV;
        }

        /* There's a lot of repetition here. This is due to the FRU having @@ -125,75 +126,75 @@ int amdgpu_fru_get_product_info(struct amdgpu_device *adev)
         * and the language field, so just start from 0xb, manufacturer size
         */
        addrptr = 0xb;
-       size = amdgpu_fru_read_eeprom(adev, addrptr, buff);
-       if (size < 1) {
-               DRM_ERROR("Failed to read FRU Manufacturer, ret:%d", size);
-               return size;
+       len = size = amdgpu_fru_read_eeprom(adev, addrptr, buff);
+       if (len < 1) {
+               DRM_ERROR("Failed to read FRU Manufacturer, ret:%d", len);
+               return -EINVAL;
        }

        /* Increment the addrptr by the size of the field, and 1 due to the
         * size field being 1 byte. This pattern continues below.
         */
        addrptr += size + 1;
-       size = amdgpu_fru_read_eeprom(adev, addrptr, buff);
-       if (size < 1) {
-               DRM_ERROR("Failed to read FRU product name, ret:%d", size);
-               return size;
+       len = size = amdgpu_fru_read_eeprom(adev, addrptr, buff);
+       if (len < 1) {
+               DRM_ERROR("Failed to read FRU product name, ret:%d", len);
+               return -EINVAL;
        }

        /* Product name should only be 32 characters. Any more,
         * and something could be wrong. Cap it at 32 to be safe
         */
-       if (size > 32) {
+       if (len >= sizeof(adev->product_name)) {
                DRM_WARN("FRU Product Number is larger than 32 characters. This is likely a mistake");
-               size = 32;
+               len = sizeof(adev->product_name) - 1;
        }
        /* Start at 2 due to buff using fields 0 and 1 for the address */
-       memcpy(adev->product_name, &buff[2], size);
-       adev->product_name[size] = '\0';
+       memcpy(adev->product_name, &buff[2], len);
+       adev->product_name[len] = '\0';

        addrptr += size + 1;
-       size = amdgpu_fru_read_eeprom(adev, addrptr, buff);
-       if (size < 1) {
-               DRM_ERROR("Failed to read FRU product number, ret:%d", size);
-               return size;
+       len = size = amdgpu_fru_read_eeprom(adev, addrptr, buff);
+       if (len < 1) {
+               DRM_ERROR("Failed to read FRU product number, ret:%d", len);
+               return -EINVAL;
        }

        /* Product number should only be 16 characters. Any more,
         * and something could be wrong. Cap it at 16 to be safe
         */
-       if (size > 16) {
+       if (len >= sizeof(adev->product_number)) {
                DRM_WARN("FRU Product Number is larger than 16 characters. This is likely a mistake");
-               size = 16;
+               len = sizeof(adev->product_number) - 1;
        }
-       memcpy(adev->product_number, &buff[2], size);
-       adev->product_number[size] = '\0';
+       memcpy(adev->product_number, &buff[2], len);
+       adev->product_number[len] = '\0';

        addrptr += size + 1;
-       size = amdgpu_fru_read_eeprom(adev, addrptr, buff);
+       len = size = amdgpu_fru_read_eeprom(adev, addrptr, buff);

-       if (size < 1) {
-               DRM_ERROR("Failed to read FRU product version, ret:%d", size);
-               return size;
+       if (len < 1) {
+               DRM_ERROR("Failed to read FRU product version, ret:%d", len);
+               return -EINVAL;
        }

        addrptr += size + 1;
-       size = amdgpu_fru_read_eeprom(adev, addrptr, buff);
+       len = size = amdgpu_fru_read_eeprom(adev, addrptr, buff);

-       if (size < 1) {
-               DRM_ERROR("Failed to read FRU serial number, ret:%d", size);
-               return size;
+       if (len < 1) {
+               DRM_ERROR("Failed to read FRU serial number, ret:%d", len);
+               return -EINVAL;
        }

        /* Serial number should only be 16 characters. Any more,
         * and something could be wrong. Cap it at 16 to be safe
         */
-       if (size > 16) {
+       if (len >= sizeof(adev->serial)) {
                DRM_WARN("FRU Serial Number is larger than 16 characters. This is likely a mistake");
-               size = 16;
+               len = sizeof(adev->serial) - 1;
        }
-       memcpy(adev->serial, &buff[2], size);
-       adev->serial[size] = '\0';
+       memcpy(adev->serial, &buff[2], len);
+       adev->serial[len] = '\0';

        return 0;
 }
--
2.25.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx