Re: [PATCH] drm/amdgpu: Add vbios info ioctl interface

Re: [PATCH] drm/amdgpu: Add vbios info ioctl interface

[Kees Cook]

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

RE: [PATCH] drm/amdgpu: Add vbios info ioctl interface

[Gu, JiaWei (Will)]

[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

Re: [PATCH] drm/amdgpu: Add vbios info ioctl interface

[Kees Cook]

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