Re: [PATCH v2] [next] drm/radeon: Replace one-element array with flexible-array member

[Kees Cook <keescook@chromium.org>]

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@gmail.com> 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).

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