On Wed, Jun 10, 2020 at 6:57 AM Quan, Evan <Evan.Quan@xxxxxxx> wrote:
>
> [AMD Official Use Only - Internal Distribution Only]
>
> Reviewed-by: Evan Quan <evan.quan@xxxxxxx>
Applied. Thanks!
Alex
>
> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> Sent: Wednesday, June 10, 2020 4:57 PM
> To: Deucher, Alexander <Alexander.Deucher@xxxxxxx>
> Cc: Koenig, Christian <Christian.Koenig@xxxxxxx>; David Airlie <airlied@xxxxxxxx>; Daniel Vetter <daniel@xxxxxxxx>; Quan, Evan <Evan.Quan@xxxxxxx>; Zhang, Hawking <Hawking.Zhang@xxxxxxx>; Kuehling, Felix <Felix.Kuehling@xxxxxxx>; Zhou1, Tao <Tao.Zhou1@xxxxxxx>; Ma, Le <Le.Ma@xxxxxxx>; Xiao, Jack <Jack.Xiao@xxxxxxx>; Kim, Jonathan <Jonathan.Kim@xxxxxxx>; Greathouse, Joseph <Joseph.Greathouse@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx; kernel-janitors@xxxxxxxxxxxxxxx
> Subject: [PATCH v2] drm/amdgpu: Fix a buffer overflow handling the serial number
>
> The comments say that the serial number is a 16-digit HEX string so the
> buffer needs to be at least 17 characters to hold the NUL terminator.
>
> The other issue is that "size" returned from sprintf() is the number of
> characters before the NUL terminator so the memcpy() wasn't copying the
> terminator. The serial number needs to be NUL terminated so that it
> doesn't lead to a read overflow in amdgpu_device_get_serial_number().
> Also it's just cleaner and faster to sprintf() directly to adev->serial[]
> instead of using a temporary buffer.
>
> Fixes: 81a16241114b ("drm/amdgpu: Add unique_id and serial_number for Arcturus v3")
> Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> ---
> v2: Change adev->serial. The original patch would have just moved the
> overflow until amdgpu_device_get_serial_number() is called.
>
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu.h | 2 +-
> drivers/gpu/drm/amd/powerplay/arcturus_ppt.c | 6 ++----
> 2 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 135530286f34f..905cf0bac100c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -986,7 +986,7 @@ struct amdgpu_device {
> /* Chip product information */
> charproduct_number[16];
> charproduct_name[32];
> -charserial[16];
> +charserial[20];
>
> struct amdgpu_autodumpautodump;
>
> diff --git a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> index df7b408319f76..d58146a5fa21d 100644
> --- a/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> +++ b/drivers/gpu/drm/amd/powerplay/arcturus_ppt.c
> @@ -2265,8 +2265,7 @@ static void arcturus_i2c_eeprom_control_fini(struct i2c_adapter *control)
> static void arcturus_get_unique_id(struct smu_context *smu)
> {
> struct amdgpu_device *adev = smu->adev;
> -uint32_t top32, bottom32, smu_version, size;
> -char sn[16];
> +uint32_t top32, bottom32, smu_version;
> uint64_t id;
>
> if (smu_get_smc_version(smu, NULL, &smu_version)) {
> @@ -2289,8 +2288,7 @@ static void arcturus_get_unique_id(struct smu_context *smu)
> /* For Arcturus-and-later, unique_id == serial_number, so convert it to a
> * 16-digit HEX string for convenience and backwards-compatibility
> */
> -size = sprintf(sn, "%llx", id);
> -memcpy(adev->serial, &sn, size);
> +sprintf(adev->serial, "%llx", id);
> }
>
> static bool arcturus_is_baco_supported(struct smu_context *smu)
> --
> 2.26.2
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
