| Message ID | Y1yetX1CHsr+fibp@mail.google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,next] drm/radeon: Replace one-element array with flexible-array member | expand |
On Sat, Oct 29, 2022 at 04:32:05PM +1300, Paulo Miguel Almeida 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. Thanks for checking it! > > 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]. > > Link: https://github.com/KSPP/linux/issues/79 > Link: https://github.com/KSPP/linux/issues/239 > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836 [1] > > Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> Reviewed-by: Kees Cook <keescook@chromium.org>
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 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. Alex > > Link: https://github.com/KSPP/linux/issues/79 > Link: https://github.com/KSPP/linux/issues/239 > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836 [1] > > Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> > --- > Changelog: > > v2: no binary output differences patch; report binary changes findings > on commit log. Res: Kees Cook. > > This request was made in an identical, yet different, patch but the > same feedback applies. > https://lore.kernel.org/lkml/Y1x3MtRJ8ckXxlJn@mail.google.com/ > > v1: https://lore.kernel.org/lkml/Y1trhRE3nK5iAY6q@mail.google.com/ > --- > drivers/gpu/drm/radeon/atombios.h | 2 +- > drivers/gpu/drm/radeon/radeon_atombios.c | 7 +++++-- > 2 files changed, 6 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/radeon/atombios.h b/drivers/gpu/drm/radeon/atombios.h > index da35a970fcc0..235e59b547a1 100644 > --- a/drivers/gpu/drm/radeon/atombios.h > +++ b/drivers/gpu/drm/radeon/atombios.h > @@ -3615,7 +3615,7 @@ typedef struct _ATOM_FAKE_EDID_PATCH_RECORD > { > UCHAR ucRecordType; > UCHAR ucFakeEDIDLength; > - UCHAR ucFakeEDIDString[1]; // This actually has ucFakeEdidLength elements. > + UCHAR ucFakeEDIDString[]; // This actually has ucFakeEdidLength elements. > } ATOM_FAKE_EDID_PATCH_RECORD; > > typedef struct _ATOM_PANEL_RESOLUTION_PATCH_RECORD > diff --git a/drivers/gpu/drm/radeon/radeon_atombios.c b/drivers/gpu/drm/radeon/radeon_atombios.c > index 204127bad89c..4ad5a328d920 100644 > --- a/drivers/gpu/drm/radeon/radeon_atombios.c > +++ b/drivers/gpu/drm/radeon/radeon_atombios.c > @@ -1727,8 +1727,11 @@ struct radeon_encoder_atom_dig *radeon_atombios_get_lvds_info(struct > } > } > record += fake_edid_record->ucFakeEDIDLength ? > - fake_edid_record->ucFakeEDIDLength + 2 : > - sizeof(ATOM_FAKE_EDID_PATCH_RECORD); > + struct_size(fake_edid_record, > + ucFakeEDIDString, > + fake_edid_record->ucFakeEDIDLength) : > + /* empty fake edid record must be 3 bytes long */ > + sizeof(ATOM_FAKE_EDID_PATCH_RECORD) + 1; > break; > case LCD_PANEL_RESOLUTION_RECORD_TYPE: > panel_res_record = (ATOM_PANEL_RESOLUTION_PATCH_RECORD *)record; > -- > 2.37.3 >
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 > 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. > > Alex > Hi Alex, thanks for taking the time to review this patch. I see where you are coming from and why you may be apprehensive with the approach. From my humble point of view, I think that the artificial line that separates "what we can change at will" and "what we should be extra careful not to break/confuse others" is whether the header file is part of the UAPI. Given that atombios.h isn't publicly accessible, I tend to gravitate towards the first one. > > + /* empty fake edid record must be 3 bytes long */ > + sizeof(ATOM_FAKE_EDID_PATCH_RECORD) + 1; I am assuming that this is the part of the patch that makes you feel concerned about how devs will get it (should they copy this header), is that right? If so, would a comment on the ATOM_FAKE_EDID_PATCH_RECORD struct specifying the size of the struct when empty do the trick? - Paulo A.
On Tue, Nov 1, 2022 at 5:14 PM Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> 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@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 > > 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. > > > > Alex > > > Hi Alex, thanks for taking the time to review this patch. > > I see where you are coming from and why you may be apprehensive with the > approach. From my humble point of view, I think that the artificial line > that separates "what we can change at will" and "what we should be extra > careful not to break/confuse others" is whether the header file is part > of the UAPI. Given that atombios.h isn't publicly accessible, I tend to > gravitate towards the first one. It may not be publicly accessible, but it describes a physical thing that is burned into millions of GPU boards out in the wild. There are tons of open source tools out there that take these headers from the kernel to be able to parse the date in the ROM on the GPU. If those applications sync up with the latest version of the header from the kernel, it will break their tools. The next time someone from AMD syncs up the header with the version maintained by the vbios team, the change could accidently sneak back in and break the code. It seems to me in this particular case that the header should reflect the physical layout of the ROM since that is what it describes. Alex > > > > + /* empty fake edid record must be 3 bytes long */ > > + sizeof(ATOM_FAKE_EDID_PATCH_RECORD) + 1; > > I am assuming that this is the part of the patch that makes you feel > concerned about how devs will get it (should they copy this header), > is that right? > > If so, would a comment on the ATOM_FAKE_EDID_PATCH_RECORD struct > specifying the size of the struct when empty do the trick? > > - Paulo A. >
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; ...
On Tue, Nov 1, 2022 at 5:54 PM Kees Cook <keescook@chromium.org> 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@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). 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
On Tue, Nov 01, 2022 at 06:09:16PM -0400, Alex Deucher wrote: > On Tue, Nov 1, 2022 at 5:54 PM Kees Cook <keescook@chromium.org> wrote: > > 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); But this is exactly what the code currently does, as noted in the commit log: "It's worth mentioning that doing a build before/after this patch results in no binary output differences. > 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. Right, but if true, that seems to be a distinctly separate bug fix?
On Tue, Nov 1, 2022 at 6:41 PM Kees Cook <keescook@chromium.org> wrote: > > On Tue, Nov 01, 2022 at 06:09:16PM -0400, Alex Deucher wrote: > > On Tue, Nov 1, 2022 at 5:54 PM Kees Cook <keescook@chromium.org> wrote: > > > 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); > > But this is exactly what the code currently does, as noted in the commit > log: "It's worth mentioning that doing a build before/after this patch > results in no binary output differences. > > > 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. > > Right, but if true, that seems to be a distinctly separate bug fix? You've convinced me. Applied. Thanks, Alex > > -- > Kees Cook
diff --git a/drivers/gpu/drm/radeon/atombios.h b/drivers/gpu/drm/radeon/atombios.h index da35a970fcc0..235e59b547a1 100644 --- a/drivers/gpu/drm/radeon/atombios.h +++ b/drivers/gpu/drm/radeon/atombios.h @@ -3615,7 +3615,7 @@ typedef struct _ATOM_FAKE_EDID_PATCH_RECORD { UCHAR ucRecordType; UCHAR ucFakeEDIDLength; - UCHAR ucFakeEDIDString[1]; // This actually has ucFakeEdidLength elements. + UCHAR ucFakeEDIDString[]; // This actually has ucFakeEdidLength elements. } ATOM_FAKE_EDID_PATCH_RECORD; typedef struct _ATOM_PANEL_RESOLUTION_PATCH_RECORD diff --git a/drivers/gpu/drm/radeon/radeon_atombios.c b/drivers/gpu/drm/radeon/radeon_atombios.c index 204127bad89c..4ad5a328d920 100644 --- a/drivers/gpu/drm/radeon/radeon_atombios.c +++ b/drivers/gpu/drm/radeon/radeon_atombios.c @@ -1727,8 +1727,11 @@ struct radeon_encoder_atom_dig *radeon_atombios_get_lvds_info(struct } } record += fake_edid_record->ucFakeEDIDLength ? - fake_edid_record->ucFakeEDIDLength + 2 : - sizeof(ATOM_FAKE_EDID_PATCH_RECORD); + struct_size(fake_edid_record, + ucFakeEDIDString, + fake_edid_record->ucFakeEDIDLength) : + /* empty fake edid record must be 3 bytes long */ + sizeof(ATOM_FAKE_EDID_PATCH_RECORD) + 1; break; case LCD_PANEL_RESOLUTION_RECORD_TYPE: panel_res_record = (ATOM_PANEL_RESOLUTION_PATCH_RECORD *)record;
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]. Link: https://github.com/KSPP/linux/issues/79 Link: https://github.com/KSPP/linux/issues/239 Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836 [1] Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> --- Changelog: v2: no binary output differences patch; report binary changes findings on commit log. Res: Kees Cook. This request was made in an identical, yet different, patch but the same feedback applies. https://lore.kernel.org/lkml/Y1x3MtRJ8ckXxlJn@mail.google.com/ v1: https://lore.kernel.org/lkml/Y1trhRE3nK5iAY6q@mail.google.com/ --- drivers/gpu/drm/radeon/atombios.h | 2 +- drivers/gpu/drm/radeon/radeon_atombios.c | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-)