On Tue, Nov 1, 2022 at 5:54 PM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> On Tue, Nov 01, 2022 at 10:42:14AM -0400, Alex Deucher wrote:
> > On Fri, Oct 28, 2022 at 11:32 PM Paulo Miguel Almeida
> > <paulo.miguel.almeida.rodenas@xxxxxxxxx> wrote:
> > >
> > > One-element arrays are deprecated, and we are replacing them with
> > > flexible array members instead. So, replace one-element array with
> > > flexible-array member in struct _ATOM_FAKE_EDID_PATCH_RECORD and
> > > refactor the rest of the code accordingly.
> > >
> > > It's worth mentioning that doing a build before/after this patch results
> > > in no binary output differences.
> > >
> > > This helps with the ongoing efforts to tighten the FORTIFY_SOURCE
> > > routines on memcpy() and help us make progress towards globally
> > > enabling -fstrict-flex-arrays=3 [1].
> >
> > This seems like a worthy goal, and I'm not opposed to the patch per
> > se, but it seems a bit at odds with what this header represents.
> > atombios.h represents the memory layout of the data stored in the ROM
> > on the GPU. This changes the memory layout of that ROM. We can work
>
> It doesn't though. Or maybe I'm misunderstanding what you mean.
>
> > around that in the driver code, but if someone were to take this
> > header to try and write some standalone tool or use it for something
> > else in the kernel, it would not reflect reality.
>
> Does the ROM always only have a single byte there? This seems unlikely
> given the member "ucFakeEDIDLength" (and the code below).
I'm not sure. I'm mostly concerned about this:
record +=
fake_edid_record->ucFakeEDIDLength ?
fake_edid_record->ucFakeEDIDLength + 2 :
sizeof(ATOM_FAKE_EDID_PATCH_RECORD);
Presumably the record should only exist if ucFakeEDIDLength is non 0,
but I don't know if there are some OEMs out there that just included
an empty record for some reason. Maybe the code is wrong today and
there are some OEMs that include it and the array is already size 0.
In that case, Paulo's original patches are probably more correct.
Alex
>
> The problem on the kernel side is that the code just before the patch
> context in drivers/gpu/drm/amd/amdgpu/atombios_encoders.c will become
> a problem soon:
>
> if (fake_edid_record->ucFakeEDIDLength) {
> struct edid *edid;
> int edid_size =
> max((int)EDID_LENGTH, (int)fake_edid_record->ucFakeEDIDLength);
> edid = kmalloc(edid_size, GFP_KERNEL);
> if (edid) {
> memcpy((u8 *)edid, (u8 *)&fake_edid_record->ucFakeEDIDString[0],
> fake_edid_record->ucFakeEDIDLength);
>
> if (drm_edid_is_valid(edid)) {
> ...
>
> the memcpy() from "fake_edid_record->ucFakeEDIDString" will eventually
> start to WARN, since the size of that field will be seen as a single byte
> under -fstrict-flex-arrays. At this moment, no, there's neither source
> bounds checking nor -fstrict-flex-arrays, but we're trying to clean up
> everything we can find now so that we don't have to do this all again
> later. :)
>
> -Kees
>
> P.S. Also this could be shorter with fewer casts:
>
> struct edid *edid;
> u8 edid_size =
> max_t(u8, EDID_LENGTH, fake_edid_record->ucFakeEDIDLength);
> edid = kmemdup(fake_edid_record->ucFakeEDIDString, edid_size, GFP_KERNEL);
> if (edid) {
> if (drm_edid_is_valid(edid)) {
> adev->mode_info.bios_hardcoded_edid = edid;
> ...
>
> --
> Kees Cook
