Messages in this thread |  | | Date | Wed, 14 Dec 2022 12:26:01 -0800 | From | Kees Cook <> | Subject | Re: [PATCH] [next] pcmcia: synclink_cs: replace 1-element array with flex-array member |
| |
On Thu, Dec 15, 2022 at 09:09:46AM +1300, Paulo Miguel Almeida wrote: > On Wed, Dec 14, 2022 at 11:29:37AM -0800, Kees Cook wrote: > > On Wed, Dec 14, 2022 at 09:42:00PM +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 RXBUF 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://gcc.gnu.org/bugzilla/show_bug.cgi?id=101836 [1] > > > > > > Signed-off-by: Paulo Miguel Almeida <paulo.miguel.almeida.rodenas@gmail.com> > > > --- > > > drivers/char/pcmcia/synclink_cs.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/char/pcmcia/synclink_cs.c b/drivers/char/pcmcia/synclink_cs.c > > > index b2735be81ab2..1ab2d552f498 100644 > > > --- a/drivers/char/pcmcia/synclink_cs.c > > > +++ b/drivers/char/pcmcia/synclink_cs.c > > > @@ -105,7 +105,7 @@ static MGSL_PARAMS default_params = { > > > typedef struct { > > > int count; > > > unsigned char status; > > > - char data[1]; > > > + char data[]; > > > } RXBUF; > > > > > > /* The queue of BH actions to be performed */ > > > @@ -2611,7 +2611,8 @@ static int mgslpc_proc_show(struct seq_file *m, void *v) > > > static int rx_alloc_buffers(MGSLPC_INFO *info) > > > { > > > /* each buffer has header and data */ > > > - info->rx_buf_size = sizeof(RXBUF) + info->max_frame_size; > > > + info->rx_buf_size = max(offsetof(typeof(RXBUF), data) + 1, sizeof(RXBUF)) > > > + + info->max_frame_size; > > > > It seems like there is an existing size bug here, and likely should be > > fixed separately? > > > > i.e. this was already allocating 1 byte "too much". I'd expect this > > first: > > > > - info->rx_buf_size = sizeof(RXBUF) + info->max_frame_size; > > + info->rx_buf_size = sizeof(RXBUF) - 1 + info->max_frame_size; > > > > and then the next patch: > > > > - char data[1]; > > + char data[]; > > ... > > - info->rx_buf_size = sizeof(RXBUF) - 1 + info->max_frame_size; > > + info->rx_buf_size = sizeof(RXBUF) + info->max_frame_size; > > > > The above would induce a binary output change, and the second would not. > > > > Though this results in what you had for the v2 patch (but I can't > > believe it had no binary changes...) > > > > -- > > Kees Cook > > Hi Kees, Hi Andy, Thanks for taking the time to review this patch. > > As both of you had similar points, I will reply them here. > > The reasons why it had no binary changes was because of the combination > of this 2 things: > > 1) Existing padding - so sizeof(RXBUF) returned 8 bytes in both cases. > > pahole -C RXBUF gcc/before/drivers/char/pcmcia/synclink_cs.ko > typedef struct { > int count; /* 0 4 */ > unsigned char status; /* 4 1 */ > char data[1]; /* 5 1 */ > > /* size: 8, cachelines: 1, members: 3 */ > /* padding: 2 */ > /* last cacheline: 8 bytes */ > } RXBUF; > > pahole -C RXBUF gcc/after/drivers/char/pcmcia/synclink_cs.ko > typedef struct { > int count; /* 0 4 */ > unsigned char status; /* 4 1 */ > char data[]; /* 5 0 */ > > /* size: 8, cachelines: 1, members: 3 */ > /* padding: 3 */ > /* last cacheline: 8 bytes */ > } RXBUF;
Ah-ha, now I see.
> > 2) RXBUF (as implemented now) is just like a pair of lenses from which a > developer can have access to one of the circular buffers in MGSLPC_INFO > struct called 'rx_buf'. > > 2611 static int rx_alloc_buffers(MGSLPC_INFO *info) > 2612 { > 2613 /* each buffer has header and data */ > 2614 info->rx_buf_size = sizeof(RXBUF) + info->max_frame_size; > 2615 > 2616 /* calculate total allocation size for 8 buffers */ > 2617 info->rx_buf_total_size = info->rx_buf_size * 8; > 2618 > 2619 /* limit total allocated memory */ > 2620 if (info->rx_buf_total_size > 0x10000) > 2621 info->rx_buf_total_size = 0x10000; > 2622 > 2623 /* calculate number of buffers */ > 2624 info->rx_buf_count = info->rx_buf_total_size / info->rx_buf_size; > 2625 > 2626 info->rx_buf = kmalloc(info->rx_buf_total_size, GFP_KERNEL); > > To be honest, char data[_1_] in RXBUF was never required to be there. > The code base seems to make sure that it doesn't run past its limits by > keeping track of size buffer on MGSLPC_INFO->rx_buf_size (and sometimes > RXBUF->count) > > (Addressing one point made by Andy about using of of the macros in > overflow.h) > struct_size(buf, data, 1) would return 9 bytes which could > potentially break the existing driver as it produces binary > changes.
Yeah, I think your v2 is fine. Perhaps explicitly repeat the notes about struct size padding in a v3 commit log?
-- Kees Cook
|  |