| Message ID | 20211118184352.1284792-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: omap3isp: Use struct_group() for memcpy() region | expand |
On Thu, Nov 18, 2021 at 10:43:52AM -0800, Kees Cook wrote: > 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: > > 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. > > Fixes: 378e3f81cb56 ("media: omap3isp: support 64-bit version of omap3isp_stat_data") > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > 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; Shouldn't this be data->buf = (uintptr_t)(void *)data64.buf; instead? -- Gustavo > + 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..9a6b3ed11455 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 type */, 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 type */, frame, /* no attrs */, > + __u32 buf_size; > + __u16 frame_number; > + __u16 cur_frame; > + __u16 config_counter; > + ); > }; > #endif > > -- > 2.30.2 > > > >
On Mon, Dec 13, 2021 at 05:24:16PM -0600, Gustavo A. R. Silva wrote: > On Thu, Nov 18, 2021 at 10:43:52AM -0800, Kees Cook wrote: > > 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: > > > > 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. > > > > Fixes: 378e3f81cb56 ("media: omap3isp: support 64-bit version of omap3isp_stat_data") > > Signed-off-by: Kees Cook <keescook@chromium.org> > > --- > > 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; > > Shouldn't this be > > data->buf = (uintptr_t)(void *)data64.buf; > > instead? This is already a void *: struct omap3isp_stat_data { ... void __user *buf; }; But I agree, the mix of structures in here is confusing! :) -Kees
On Wed, Dec 15, 2021 at 09:38:55AM -0800, Kees Cook wrote: > On Mon, Dec 13, 2021 at 05:24:16PM -0600, Gustavo A. R. Silva wrote: > > On Thu, Nov 18, 2021 at 10:43:52AM -0800, Kees Cook wrote: > > > 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: > > > > > > 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. > > > > > > Fixes: 378e3f81cb56 ("media: omap3isp: support 64-bit version of omap3isp_stat_data") > > > Signed-off-by: Kees Cook <keescook@chromium.org> Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org> > > > --- > > > 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; > > > > Shouldn't this be > > > > data->buf = (uintptr_t)(void *)data64.buf; > > > > instead? > > This is already a void *: > > struct omap3isp_stat_data { > ... > void __user *buf; > }; > > But I agree, the mix of structures in here is confusing! :) Yep; you're right. :) Thanks -- Gustavo
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..9a6b3ed11455 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 type */, 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 type */, frame, /* no attrs */, + __u32 buf_size; + __u16 frame_number; + __u16 cur_frame; + __u16 config_counter; + ); }; #endif
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: 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. Fixes: 378e3f81cb56 ("media: omap3isp: support 64-bit version of omap3isp_stat_data") Signed-off-by: Kees Cook <keescook@chromium.org> --- drivers/media/platform/omap3isp/ispstat.c | 5 +++-- include/uapi/linux/omap3isp.h | 21 +++++++++++++-------- 2 files changed, 16 insertions(+), 10 deletions(-)