[AMD Official Use Only - Internal Distribution Only]
Thanks for this catching Kees.
Yes it should be 20, not 16. I was not aware that serial size had been changed
from 16 to 20 in struct amdgpu_device.
Will submit a fix soon.
Best regards,
Jiawei
-----Original Message-----
From: Kees Cook <keesc...@chromium.org>
Sent: Saturday, May 8, 2021 12:28 PM
To: Gu, JiaWei (Will) <jiawei...@amd.com>; Deucher, Alexander
<alexander.deuc...@amd.com>
Cc: StDenis, Tom <tom.stde...@amd.com>; Deucher, Alexander
<alexander.deuc...@amd.com>; Christian König
<ckoenig.leichtzumer...@gmail.com>; Gu, JiaWei (Will) <jiawei...@amd.com>;
amd-gfx@lists.freedesktop.org; Nieto, David M <david.ni...@amd.com>;
linux-n...@vger.kernel.org
Subject: Re: [PATCH] drm/amdgpu: Add vbios info ioctl interface
Hi!
This patch needs some fixing.
On Thu, Apr 22, 2021 at 10:34:48AM +0800, Jiawei Gu wrote:
> + case AMDGPU_INFO_VBIOS_INFO: {
> + struct drm_amdgpu_info_vbios vbios_info = {};
> + struct atom_context *atom_context;
> +
> + atom_context = adev->mode_info.atom_context;
> + memcpy(vbios_info.name, atom_context->name,
> sizeof(atom_context->name));
> + vbios_info.dbdf = PCI_DEVID(adev->pdev->bus->number,
> adev->pdev->devfn);
> + memcpy(vbios_info.vbios_pn, atom_context->vbios_pn,
> sizeof(atom_context->vbios_pn));
> + vbios_info.version = atom_context->version;
> + memcpy(vbios_info.date, atom_context->date,
> sizeof(atom_context->date));
> + memcpy(vbios_info.serial, adev->serial,
> sizeof(adev->serial));
This writes beyond the end of vbios_info.serial.
> + vbios_info.dev_id = adev->pdev->device;
> + vbios_info.rev_id = adev->pdev->revision;
> + vbios_info.sub_dev_id = atom_context->sub_dev_id;
> + vbios_info.sub_ved_id = atom_context->sub_ved_id;
Though it gets "repaired" by these writes.
> +
> + return copy_to_user(out, &vbios_info,
> + min((size_t)size,
> sizeof(vbios_info))) ? -EFAULT : 0;
> + }
sizeof(adev->serial) != sizeof(vbios_info.serial)
adev is struct amdgpu_device:
struct amdgpu_device {
...
char serial[20];
> +struct drm_amdgpu_info_vbios {
> [...]
> + __u8 serial[16];
> + __u32 dev_id;
> + __u32 rev_id;
> + __u32 sub_dev_id;
> + __u32 sub_ved_id;
> +};
Is there a truncation issue (20 vs 16) and is this intended to be a
NUL-terminated string?
--
Kees Cook
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
