* Re: [PATCH] drm/amdgpu: Add vbios info ioctl interface [not found] <20210422023448.24689-1-Jiawei.Gu@amd.com> @ 2021-05-08 4:28 ` Kees Cook 2021-05-08 6:01 ` Gu, JiaWei (Will) 0 siblings, 1 reply; 3+ messages in thread From: Kees Cook @ 2021-05-08 4:28 UTC (permalink / raw) To: Jiawei Gu, Deucher, Alexander Cc: StDenis, Tom, Deucher, Alexander, Christian König, Gu, JiaWei (Will), amd-gfx@lists.freedesktop.org, Nieto, David M, linux-next 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH] drm/amdgpu: Add vbios info ioctl interface 2021-05-08 4:28 ` [PATCH] drm/amdgpu: Add vbios info ioctl interface Kees Cook @ 2021-05-08 6:01 ` Gu, JiaWei (Will) 2021-05-08 9:51 ` Kees Cook 0 siblings, 1 reply; 3+ messages in thread From: Gu, JiaWei (Will) @ 2021-05-08 6:01 UTC (permalink / raw) To: Kees Cook, Deucher, Alexander Cc: StDenis, Tom, Deucher, Alexander, Christian König, amd-gfx@lists.freedesktop.org, Nieto, David M, linux-next@vger.kernel.org [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 <keescook@chromium.org> Sent: Saturday, May 8, 2021 12:28 PM To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com> Cc: StDenis, Tom <Tom.StDenis@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Christian König <ckoenig.leichtzumerken@gmail.com>; Gu, JiaWei (Will) <JiaWei.Gu@amd.com>; amd-gfx@lists.freedesktop.org; Nieto, David M <David.Nieto@amd.com>; linux-next@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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] drm/amdgpu: Add vbios info ioctl interface 2021-05-08 6:01 ` Gu, JiaWei (Will) @ 2021-05-08 9:51 ` Kees Cook 0 siblings, 0 replies; 3+ messages in thread From: Kees Cook @ 2021-05-08 9:51 UTC (permalink / raw) To: Gu, JiaWei (Will) Cc: Deucher, Alexander, StDenis, Tom, Christian König, amd-gfx@lists.freedesktop.org, Nieto, David M, linux-next@vger.kernel.org On Sat, May 08, 2021 at 06:01:23AM +0000, Gu, JiaWei (Will) wrote: > [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. You might want to add a BUILD_BUG_ON() to keep those in sync, especially since it's about to be UAPI. -Kees > > Best regards, > Jiawei > > > -----Original Message----- > From: Kees Cook <keescook@chromium.org> > Sent: Saturday, May 8, 2021 12:28 PM > To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com> > Cc: StDenis, Tom <Tom.StDenis@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com>; Christian König <ckoenig.leichtzumerken@gmail.com>; Gu, JiaWei (Will) <JiaWei.Gu@amd.com>; amd-gfx@lists.freedesktop.org; Nieto, David M <David.Nieto@amd.com>; linux-next@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 -- Kees Cook ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-05-08 9:51 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20210422023448.24689-1-Jiawei.Gu@amd.com>
2021-05-08 4:28 ` [PATCH] drm/amdgpu: Add vbios info ioctl interface Kees Cook
2021-05-08 6:01 ` Gu, JiaWei (Will)
2021-05-08 9:51 ` Kees Cook
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).