<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<style type="text/css" style="display:none;"> P {margin-top:0;margin-bottom:0;} </style>
</head>
<body dir="ltr">
<p style="font-family:Arial;font-size:10pt;color:#317100;margin:15pt;" align="Left">
[AMD Public Use]<br>
</p>
<br>
<div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0); background-color: rgb(255, 255, 255);">
The full vbios versioning information consists of the version (numeric), build date and the name... I this this interface exposes only the name.</div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0); background-color: rgb(255, 255, 255);">
<br>
</div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0); background-color: rgb(255, 255, 255);">
Should we expose those on different sysfs files or just combine all of them in a single file?</div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0); background-color: rgb(255, 255, 255);">
<br>
</div>
<div style="font-family: Calibri, Arial, Helvetica, sans-serif; font-size: 12pt; color: rgb(0, 0, 0); background-color: rgb(255, 255, 255);">
David</div>
<div id="appendonsend"></div>
<hr style="display:inline-block;width:98%" tabindex="-1">
<div id="divRplyFwdMsg" dir="ltr"><font face="Calibri, sans-serif" style="font-size:11pt" color="#000000"><b>From:</b> Kees Cook <keescook@chromium.org><br>
<b>Sent:</b> Saturday, May 8, 2021 2:51 AM<br>
<b>To:</b> Gu, JiaWei (Will) <JiaWei.Gu@amd.com><br>
<b>Cc:</b> Deucher, Alexander <Alexander.Deucher@amd.com>; StDenis, Tom <Tom.StDenis@amd.com>; Christian König <ckoenig.leichtzumerken@gmail.com>; amd-gfx@lists.freedesktop.org <amd-gfx@lists.freedesktop.org>; Nieto, David M <David.Nieto@amd.com>; linux-next@vger.kernel.org
<linux-next@vger.kernel.org><br>
<b>Subject:</b> Re: [PATCH] drm/amdgpu: Add vbios info ioctl interface</font>
<div> </div>
</div>
<div class="BodyFragment"><font size="2"><span style="font-size:11pt;">
<div class="PlainText">On Sat, May 08, 2021 at 06:01:23AM +0000, Gu, JiaWei (Will) wrote:<br>
> [AMD Official Use Only - Internal Distribution Only]<br>
> <br>
> Thanks for this catching Kees.<br>
> <br>
> 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.<br>
> Will submit a fix soon.<br>
<br>
You might want to add a BUILD_BUG_ON() to keep those in sync, especially<br>
since it's about to be UAPI.<br>
<br>
-Kees<br>
<br>
> <br>
> Best regards,<br>
> Jiawei<br>
> <br>
> <br>
> -----Original Message-----<br>
> From: Kees Cook <keescook@chromium.org> <br>
> Sent: Saturday, May 8, 2021 12:28 PM<br>
> To: Gu, JiaWei (Will) <JiaWei.Gu@amd.com>; Deucher, Alexander <Alexander.Deucher@amd.com><br>
> 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<br>
> Subject: Re: [PATCH] drm/amdgpu: Add vbios info ioctl interface<br>
> <br>
> Hi!<br>
> <br>
> This patch needs some fixing.<br>
> <br>
> On Thu, Apr 22, 2021 at 10:34:48AM +0800, Jiawei Gu wrote:<br>
> > + case AMDGPU_INFO_VBIOS_INFO: {<br>
> > + struct drm_amdgpu_info_vbios vbios_info = {};<br>
> > + struct atom_context *atom_context;<br>
> > +<br>
> > + atom_context = adev->mode_info.atom_context;<br>
> > + memcpy(vbios_info.name, atom_context->name, sizeof(atom_context->name));<br>
> > + vbios_info.dbdf = PCI_DEVID(adev->pdev->bus->number, adev->pdev->devfn);<br>
> > + memcpy(vbios_info.vbios_pn, atom_context->vbios_pn, sizeof(atom_context->vbios_pn));<br>
> > + vbios_info.version = atom_context->version;<br>
> > + memcpy(vbios_info.date, atom_context->date, sizeof(atom_context->date));<br>
> > + memcpy(vbios_info.serial, adev->serial, sizeof(adev->serial));<br>
> <br>
> This writes beyond the end of vbios_info.serial.<br>
> <br>
> > + vbios_info.dev_id = adev->pdev->device;<br>
> > + vbios_info.rev_id = adev->pdev->revision;<br>
> > + vbios_info.sub_dev_id = atom_context->sub_dev_id;<br>
> > + vbios_info.sub_ved_id = atom_context->sub_ved_id;<br>
> <br>
> Though it gets "repaired" by these writes.<br>
> <br>
> > +<br>
> > + return copy_to_user(out, &vbios_info,<br>
> > + min((size_t)size, sizeof(vbios_info))) ? -EFAULT : 0;<br>
> > + }<br>
> <br>
> sizeof(adev->serial) != sizeof(vbios_info.serial)<br>
> <br>
> adev is struct amdgpu_device:<br>
> <br>
> struct amdgpu_device {<br>
> ...<br>
> char serial[20];<br>
> <br>
> <br>
> > +struct drm_amdgpu_info_vbios {<br>
> > [...]<br>
> > + __u8 serial[16];<br>
> > + __u32 dev_id;<br>
> > + __u32 rev_id;<br>
> > + __u32 sub_dev_id;<br>
> > + __u32 sub_ved_id;<br>
> > +};<br>
> <br>
> Is there a truncation issue (20 vs 16) and is this intended to be a NUL-terminated string?<br>
> <br>
> --<br>
> Kees Cook<br>
<br>
-- <br>
Kees Cook<br>
</div>
</span></font></div>
</div>
</body>
</html>