i915/gvt: Replace one-element array with flexible-array member
authorPaulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
Tue, 20 Dec 2022 03:41:15 +0000 (16:41 +1300)
committerKees Cook <keescook@chromium.org>
Fri, 27 Jan 2023 19:42:56 +0000 (11:42 -0800)
commit16a738f2f6b31f9edf0691e0f357539f7ac66662
tree15c5489d4480f4ab602a6d12da0cef6f6feff546
parent4076ea2419cf15bc1e1580f8b24ddf675fbdb02c
i915/gvt: Replace one-element array with flexible-array member

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 gvt_firmware_header and refactor the
rest of the code accordingly.

Additionally, previous implementation was allocating 8 bytes more than
required to represent firmware_header + cfg_space data + mmio data.

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].

To make reviewing this patch easier, I'm pasting before/after struct
sizes.

pahole -C gvt_firmware_header before/drivers/gpu/drm/i915/gvt/firmware.o
struct gvt_firmware_header {
u64                        magic;                /*     0     8 */
u32                        crc32;                /*     8     4 */
u32                        version;              /*    12     4 */
u64                        cfg_space_size;       /*    16     8 */
u64                        cfg_space_offset;     /*    24     8 */
u64                        mmio_size;            /*    32     8 */
u64                        mmio_offset;          /*    40     8 */
unsigned char              data[1];              /*    48     1 */

/* size: 56, cachelines: 1, members: 8 */
/* padding: 7 */
/* last cacheline: 56 bytes */
};

pahole -C gvt_firmware_header after/drivers/gpu/drm/i915/gvt/firmware.o
struct gvt_firmware_header {
u64                        magic;                /*     0     8 */
u32                        crc32;                /*     8     4 */
u32                        version;              /*    12     4 */
u64                        cfg_space_size;       /*    16     8 */
u64                        cfg_space_offset;     /*    24     8 */
u64                        mmio_size;            /*    32     8 */
u64                        mmio_offset;          /*    40     8 */
unsigned char              data[];               /*    48     0 */

/* size: 48, cachelines: 1, members: 8 */
/* last cacheline: 48 bytes */
};

As you can see the additional byte of the fake-flexible array (data[1])
forced the compiler to pad the struct but those bytes aren't actually used
as first & last bytes (of both cfg_space and mmio) are controlled by the
<>_size and <>_offset members present in the gvt_firmware_header struct.

Link: https://github.com/KSPP/linux/issues/79
Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836
Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com>
Reviewed-by: Zhenyu Wang <zhenyuw@linux.intel.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
Link: https://lore.kernel.org/r/Y6Eu2604cqtryP4g@mail.google.com
drivers/gpu/drm/i915/gvt/firmware.c