Em Mon, 24 Jan 2022 09:29:52 -0800 Kees Cook <keescook@xxxxxxxxxxxx> escreveu: > In preparation for FORTIFY_SOURCE performing compile-time and run-time > field bounds checking for memcpy(), memmove(), and memset(), avoid > intentionally writing across neighboring fields. Wrap the target region > in struct_group(). This additionally fixes a theoretical misalignment > of the copy (since the size of "buf" changes between 64-bit and 32-bit, > but this is likely never built for 64-bit). > FWIW, I think this code is totally broken on 64-bit (which appears to > not be a "real" build configuration): it would either always fail (with > an uninitialized data->buf_size) or would cause corruption in userspace > due to the copy_to_user() in the call path against an uninitialized > data->buf value: It doesn't matter. This driver is specific for TI OMAP3 SoC, which is Cortex-A8 (32-bits). It only builds on 64 bit due to COMPILE_TEST. Regards, Mauro > > omap3isp_stat_request_statistics_time32(...) > struct omap3isp_stat_data data64; > ... > omap3isp_stat_request_statistics(stat, &data64); > > int omap3isp_stat_request_statistics(struct ispstat *stat, > struct omap3isp_stat_data *data) > ... > buf = isp_stat_buf_get(stat, data); > > static struct ispstat_buffer *isp_stat_buf_get(struct ispstat *stat, > struct omap3isp_stat_data *data) > ... > if (buf->buf_size > data->buf_size) { > ... > return ERR_PTR(-EINVAL); > } > ... > rval = copy_to_user(data->buf, > buf->virt_addr, > buf->buf_size); > > Regardless, additionally initialize data64 to be zero-filled to avoid > undefined behavior. > > Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> > Cc: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx> > Cc: Arnd Bergmann <arnd@xxxxxxxx> > Cc: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> > Cc: linux-media@xxxxxxxxxxxxxxx > Fixes: 378e3f81cb56 ("media: omap3isp: support 64-bit version of omap3isp_stat_data") > Cc: stable@xxxxxxxxxxxxxxx > Reviewed-by: Gustavo A. R. Silva <gustavoars@xxxxxxxxxx> > Link: https://lore.kernel.org/lkml/20211215220505.GB21862@embeddedor > Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx> > --- > I will carry this in my tree unless someone else wants to pick it up. It's > one of the last remaining clean-ups needed for the next step in memcpy() > hardening. > --- > drivers/media/platform/omap3isp/ispstat.c | 5 +++-- > include/uapi/linux/omap3isp.h | 21 +++++++++++++-------- > 2 files changed, 16 insertions(+), 10 deletions(-) > > diff --git a/drivers/media/platform/omap3isp/ispstat.c b/drivers/media/platform/omap3isp/ispstat.c > index 5b9b57f4d9bf..68cf68dbcace 100644 > --- a/drivers/media/platform/omap3isp/ispstat.c > +++ b/drivers/media/platform/omap3isp/ispstat.c > @@ -512,7 +512,7 @@ int omap3isp_stat_request_statistics(struct ispstat *stat, > int omap3isp_stat_request_statistics_time32(struct ispstat *stat, > struct omap3isp_stat_data_time32 *data) > { > - struct omap3isp_stat_data data64; > + struct omap3isp_stat_data data64 = { }; > int ret; > > ret = omap3isp_stat_request_statistics(stat, &data64); > @@ -521,7 +521,8 @@ int omap3isp_stat_request_statistics_time32(struct ispstat *stat, > > data->ts.tv_sec = data64.ts.tv_sec; > data->ts.tv_usec = data64.ts.tv_usec; > - memcpy(&data->buf, &data64.buf, sizeof(*data) - sizeof(data->ts)); > + data->buf = (uintptr_t)data64.buf; > + memcpy(&data->frame, &data64.frame, sizeof(data->frame)); > > return 0; > } > diff --git a/include/uapi/linux/omap3isp.h b/include/uapi/linux/omap3isp.h > index 87b55755f4ff..d9db7ad43890 100644 > --- a/include/uapi/linux/omap3isp.h > +++ b/include/uapi/linux/omap3isp.h > @@ -162,6 +162,7 @@ struct omap3isp_h3a_aewb_config { > * struct omap3isp_stat_data - Statistic data sent to or received from user > * @ts: Timestamp of returned framestats. > * @buf: Pointer to pass to user. > + * @buf_size: Size of buffer. > * @frame_number: Frame number of requested stats. > * @cur_frame: Current frame number being processed. > * @config_counter: Number of the configuration associated with the data. > @@ -176,10 +177,12 @@ struct omap3isp_stat_data { > struct timeval ts; > #endif > void __user *buf; > - __u32 buf_size; > - __u16 frame_number; > - __u16 cur_frame; > - __u16 config_counter; > + __struct_group(/* no tag */, frame, /* no attrs */, > + __u32 buf_size; > + __u16 frame_number; > + __u16 cur_frame; > + __u16 config_counter; > + ); > }; > > #ifdef __KERNEL__ > @@ -189,10 +192,12 @@ struct omap3isp_stat_data_time32 { > __s32 tv_usec; > } ts; > __u32 buf; > - __u32 buf_size; > - __u16 frame_number; > - __u16 cur_frame; > - __u16 config_counter; > + __struct_group(/* no tag */, frame, /* no attrs */, > + __u32 buf_size; > + __u16 frame_number; > + __u16 cur_frame; > + __u16 config_counter; > + ); > }; > #endif > Thanks, Mauro