* [PATCH] media: omap3isp: Extract struct group for memcpy() region
@ 2021-06-16 18:59 Kees Cook
2021-06-16 19:43 ` Laurent Pinchart
0 siblings, 1 reply; 5+ messages in thread
From: Kees Cook @ 2021-06-16 18:59 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Kees Cook, Arnd Bergmann, Gustavo A. R. Silva, Laurent Pinchart,
Sakari Ailus, linux-kernel, linux-media
Avoid writing past the end of a structure member by wrapping the target
region in a common named structure. This additionally fixes a
misalignment of the copy (since the size of "buf" changes between 64-bit
and 32-bit).
I actually think this code is completely unused in the real world:
I don't think it could have ever worked, as 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 | 44 +++++++++++++++++------
2 files changed, 36 insertions(+), 13 deletions(-)
diff --git a/drivers/media/platform/omap3isp/ispstat.c b/drivers/media/platform/omap3isp/ispstat.c
index 5b9b57f4d9bf..ea8222fed38e 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.buf, sizeof(data->frame));
return 0;
}
diff --git a/include/uapi/linux/omap3isp.h b/include/uapi/linux/omap3isp.h
index 87b55755f4ff..0a16af91621f 100644
--- a/include/uapi/linux/omap3isp.h
+++ b/include/uapi/linux/omap3isp.h
@@ -159,13 +159,25 @@ 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.
+ * struct omap3isp_stat_frame - Statistic data without timestamp nor pointer.
+ * @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.
*/
+struct omap3isp_stat_frame {
+ __u32 buf_size;
+ __u16 frame_number;
+ __u16 cur_frame;
+ __u16 config_counter;
+};
+
+/**
+ * struct omap3isp_stat_data - Statistic data sent to or received from user
+ * @ts: Timestamp of returned framestats.
+ * @buf: Pointer to pass to user.
+ * @frame: Statistic data for frame.
+ */
struct omap3isp_stat_data {
#ifdef __KERNEL__
struct {
@@ -176,10 +188,15 @@ struct omap3isp_stat_data {
struct timeval ts;
#endif
void __user *buf;
- __u32 buf_size;
- __u16 frame_number;
- __u16 cur_frame;
- __u16 config_counter;
+ union {
+ struct {
+ __u32 buf_size;
+ __u16 frame_number;
+ __u16 cur_frame;
+ __u16 config_counter;
+ };
+ struct omap3isp_stat_frame frame;
+ };
};
#ifdef __KERNEL__
@@ -189,10 +206,15 @@ struct omap3isp_stat_data_time32 {
__s32 tv_usec;
} ts;
__u32 buf;
- __u32 buf_size;
- __u16 frame_number;
- __u16 cur_frame;
- __u16 config_counter;
+ union {
+ struct {
+ __u32 buf_size;
+ __u16 frame_number;
+ __u16 cur_frame;
+ __u16 config_counter;
+ };
+ struct omap3isp_stat_frame frame;
+ };
};
#endif
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] media: omap3isp: Extract struct group for memcpy() region 2021-06-16 18:59 [PATCH] media: omap3isp: Extract struct group for memcpy() region Kees Cook @ 2021-06-16 19:43 ` Laurent Pinchart 2021-06-17 4:22 ` Kees Cook 0 siblings, 1 reply; 5+ messages in thread From: Laurent Pinchart @ 2021-06-16 19:43 UTC (permalink / raw) To: Kees Cook Cc: Mauro Carvalho Chehab, Arnd Bergmann, Gustavo A. R. Silva, Sakari Ailus, linux-kernel, linux-media Hi Kees, Thank you for the patch. On Wed, Jun 16, 2021 at 11:59:38AM -0700, Kees Cook wrote: > Avoid writing past the end of a structure member by wrapping the target > region in a common named structure. This additionally fixes a > misalignment of the copy (since the size of "buf" changes between 64-bit > and 32-bit). Could you have been mislead by the data64 name ? The difference between omap3isp_stat_data_time and omap3isp_stat_data_time32 is the size of the ts field, using 32-bit timestamps with legacy userspace, and 64-bit timestamps with more recent userspace. In both cases we're dealing with a 32-bit platform, as the omap3isp is not used in any 64-bit ARM SoC. The size of void __user *buf is thus 4 bytes in all cases, as is __u32 buf. > I actually think this code is completely unused in the real world: > I don't think it could have ever worked, as 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 | 44 +++++++++++++++++------ > 2 files changed, 36 insertions(+), 13 deletions(-) > > diff --git a/drivers/media/platform/omap3isp/ispstat.c b/drivers/media/platform/omap3isp/ispstat.c > index 5b9b57f4d9bf..ea8222fed38e 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.buf, sizeof(data->frame)); > > return 0; > } > diff --git a/include/uapi/linux/omap3isp.h b/include/uapi/linux/omap3isp.h > index 87b55755f4ff..0a16af91621f 100644 > --- a/include/uapi/linux/omap3isp.h > +++ b/include/uapi/linux/omap3isp.h > @@ -159,13 +159,25 @@ 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. > + * struct omap3isp_stat_frame - Statistic data without timestamp nor pointer. > + * @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. > */ > +struct omap3isp_stat_frame { > + __u32 buf_size; > + __u16 frame_number; > + __u16 cur_frame; > + __u16 config_counter; > +}; > + > +/** > + * struct omap3isp_stat_data - Statistic data sent to or received from user > + * @ts: Timestamp of returned framestats. > + * @buf: Pointer to pass to user. > + * @frame: Statistic data for frame. > + */ > struct omap3isp_stat_data { > #ifdef __KERNEL__ > struct { > @@ -176,10 +188,15 @@ struct omap3isp_stat_data { > struct timeval ts; > #endif > void __user *buf; > - __u32 buf_size; > - __u16 frame_number; > - __u16 cur_frame; > - __u16 config_counter; > + union { > + struct { > + __u32 buf_size; > + __u16 frame_number; > + __u16 cur_frame; > + __u16 config_counter; > + }; > + struct omap3isp_stat_frame frame; > + }; > }; > > #ifdef __KERNEL__ > @@ -189,10 +206,15 @@ struct omap3isp_stat_data_time32 { > __s32 tv_usec; > } ts; > __u32 buf; > - __u32 buf_size; > - __u16 frame_number; > - __u16 cur_frame; > - __u16 config_counter; > + union { > + struct { > + __u32 buf_size; > + __u16 frame_number; > + __u16 cur_frame; > + __u16 config_counter; > + }; > + struct omap3isp_stat_frame frame; > + }; > }; > #endif > -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: omap3isp: Extract struct group for memcpy() region 2021-06-16 19:43 ` Laurent Pinchart @ 2021-06-17 4:22 ` Kees Cook 2021-06-23 22:12 ` Laurent Pinchart 0 siblings, 1 reply; 5+ messages in thread From: Kees Cook @ 2021-06-17 4:22 UTC (permalink / raw) To: Laurent Pinchart Cc: Mauro Carvalho Chehab, Arnd Bergmann, Gustavo A. R. Silva, Sakari Ailus, linux-kernel, linux-media On Wed, Jun 16, 2021 at 10:43:03PM +0300, Laurent Pinchart wrote: > Hi Kees, > > Thank you for the patch. > > On Wed, Jun 16, 2021 at 11:59:38AM -0700, Kees Cook wrote: > > Avoid writing past the end of a structure member by wrapping the target > > region in a common named structure. This additionally fixes a > > misalignment of the copy (since the size of "buf" changes between 64-bit > > and 32-bit). > > Could you have been mislead by the data64 name ? The difference between > omap3isp_stat_data_time and omap3isp_stat_data_time32 is the size of the > ts field, using 32-bit timestamps with legacy userspace, and 64-bit > timestamps with more recent userspace. In both cases we're dealing with > a 32-bit platform, as the omap3isp is not used in any 64-bit ARM SoC. > The size of void __user *buf is thus 4 bytes in all cases, as is __u32 > buf. Ah, yes, that's true. I was hitting this on arm64 builds (CONFIG_COMPILE_TEST) where __user *buf is 64-bit. So, the "additionally fixes" bit above is misleading in the sense that nothing was ever built in the real world like that. The patch still fixes the compile-time warnings, though. However, I don't think anything actually uses any of this code regardless. ;) -Kees > > > I actually think this code is completely unused in the real world: > > I don't think it could have ever worked, as 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 | 44 +++++++++++++++++------ > > 2 files changed, 36 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/media/platform/omap3isp/ispstat.c b/drivers/media/platform/omap3isp/ispstat.c > > index 5b9b57f4d9bf..ea8222fed38e 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.buf, sizeof(data->frame)); > > > > return 0; > > } > > diff --git a/include/uapi/linux/omap3isp.h b/include/uapi/linux/omap3isp.h > > index 87b55755f4ff..0a16af91621f 100644 > > --- a/include/uapi/linux/omap3isp.h > > +++ b/include/uapi/linux/omap3isp.h > > @@ -159,13 +159,25 @@ 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. > > + * struct omap3isp_stat_frame - Statistic data without timestamp nor pointer. > > + * @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. > > */ > > +struct omap3isp_stat_frame { > > + __u32 buf_size; > > + __u16 frame_number; > > + __u16 cur_frame; > > + __u16 config_counter; > > +}; > > + > > +/** > > + * struct omap3isp_stat_data - Statistic data sent to or received from user > > + * @ts: Timestamp of returned framestats. > > + * @buf: Pointer to pass to user. > > + * @frame: Statistic data for frame. > > + */ > > struct omap3isp_stat_data { > > #ifdef __KERNEL__ > > struct { > > @@ -176,10 +188,15 @@ struct omap3isp_stat_data { > > struct timeval ts; > > #endif > > void __user *buf; > > - __u32 buf_size; > > - __u16 frame_number; > > - __u16 cur_frame; > > - __u16 config_counter; > > + union { > > + struct { > > + __u32 buf_size; > > + __u16 frame_number; > > + __u16 cur_frame; > > + __u16 config_counter; > > + }; > > + struct omap3isp_stat_frame frame; > > + }; > > }; > > > > #ifdef __KERNEL__ > > @@ -189,10 +206,15 @@ struct omap3isp_stat_data_time32 { > > __s32 tv_usec; > > } ts; > > __u32 buf; > > - __u32 buf_size; > > - __u16 frame_number; > > - __u16 cur_frame; > > - __u16 config_counter; > > + union { > > + struct { > > + __u32 buf_size; > > + __u16 frame_number; > > + __u16 cur_frame; > > + __u16 config_counter; > > + }; > > + struct omap3isp_stat_frame frame; > > + }; > > }; > > #endif > > > > -- > Regards, > > Laurent Pinchart -- Kees Cook ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: omap3isp: Extract struct group for memcpy() region 2021-06-17 4:22 ` Kees Cook @ 2021-06-23 22:12 ` Laurent Pinchart 2021-06-23 22:53 ` Kees Cook 0 siblings, 1 reply; 5+ messages in thread From: Laurent Pinchart @ 2021-06-23 22:12 UTC (permalink / raw) To: Kees Cook Cc: Mauro Carvalho Chehab, Arnd Bergmann, Gustavo A. R. Silva, Sakari Ailus, linux-kernel, linux-media Hi Kees, On Wed, Jun 16, 2021 at 09:22:23PM -0700, Kees Cook wrote: > On Wed, Jun 16, 2021 at 10:43:03PM +0300, Laurent Pinchart wrote: > > On Wed, Jun 16, 2021 at 11:59:38AM -0700, Kees Cook wrote: > > > Avoid writing past the end of a structure member by wrapping the target > > > region in a common named structure. This additionally fixes a > > > misalignment of the copy (since the size of "buf" changes between 64-bit > > > and 32-bit). > > > > Could you have been mislead by the data64 name ? The difference between > > omap3isp_stat_data_time and omap3isp_stat_data_time32 is the size of the > > ts field, using 32-bit timestamps with legacy userspace, and 64-bit > > timestamps with more recent userspace. In both cases we're dealing with > > a 32-bit platform, as the omap3isp is not used in any 64-bit ARM SoC. > > The size of void __user *buf is thus 4 bytes in all cases, as is __u32 > > buf. > > Ah, yes, that's true. I was hitting this on arm64 builds > (CONFIG_COMPILE_TEST) where __user *buf is 64-bit. So, the "additionally > fixes" bit above is misleading in the sense that nothing was ever built > in the real world like that. > > The patch still fixes the compile-time warnings, though. I What's the compile-time warning ? I tried compiling the driver for ARM64 and didn't notice any. > However, I don't think anything actually uses any of this code > regardless. ;) > > > > I actually think this code is completely unused in the real world: > > > I don't think it could have ever worked, as 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 | 44 +++++++++++++++++------ > > > 2 files changed, 36 insertions(+), 13 deletions(-) > > > > > > diff --git a/drivers/media/platform/omap3isp/ispstat.c b/drivers/media/platform/omap3isp/ispstat.c > > > index 5b9b57f4d9bf..ea8222fed38e 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.buf, sizeof(data->frame)); > > > > > > return 0; > > > } > > > diff --git a/include/uapi/linux/omap3isp.h b/include/uapi/linux/omap3isp.h > > > index 87b55755f4ff..0a16af91621f 100644 > > > --- a/include/uapi/linux/omap3isp.h > > > +++ b/include/uapi/linux/omap3isp.h > > > @@ -159,13 +159,25 @@ 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. > > > + * struct omap3isp_stat_frame - Statistic data without timestamp nor pointer. > > > + * @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. > > > */ > > > +struct omap3isp_stat_frame { > > > + __u32 buf_size; > > > + __u16 frame_number; > > > + __u16 cur_frame; > > > + __u16 config_counter; > > > +}; > > > + > > > +/** > > > + * struct omap3isp_stat_data - Statistic data sent to or received from user > > > + * @ts: Timestamp of returned framestats. > > > + * @buf: Pointer to pass to user. > > > + * @frame: Statistic data for frame. > > > + */ > > > struct omap3isp_stat_data { > > > #ifdef __KERNEL__ > > > struct { > > > @@ -176,10 +188,15 @@ struct omap3isp_stat_data { > > > struct timeval ts; > > > #endif > > > void __user *buf; > > > - __u32 buf_size; > > > - __u16 frame_number; > > > - __u16 cur_frame; > > > - __u16 config_counter; > > > + union { > > > + struct { > > > + __u32 buf_size; > > > + __u16 frame_number; > > > + __u16 cur_frame; > > > + __u16 config_counter; > > > + }; > > > + struct omap3isp_stat_frame frame; > > > + }; > > > }; > > > > > > #ifdef __KERNEL__ > > > @@ -189,10 +206,15 @@ struct omap3isp_stat_data_time32 { > > > __s32 tv_usec; > > > } ts; > > > __u32 buf; > > > - __u32 buf_size; > > > - __u16 frame_number; > > > - __u16 cur_frame; > > > - __u16 config_counter; > > > + union { > > > + struct { > > > + __u32 buf_size; > > > + __u16 frame_number; > > > + __u16 cur_frame; > > > + __u16 config_counter; > > > + }; > > > + struct omap3isp_stat_frame frame; > > > + }; > > > }; > > > #endif > > > -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] media: omap3isp: Extract struct group for memcpy() region 2021-06-23 22:12 ` Laurent Pinchart @ 2021-06-23 22:53 ` Kees Cook 0 siblings, 0 replies; 5+ messages in thread From: Kees Cook @ 2021-06-23 22:53 UTC (permalink / raw) To: Laurent Pinchart Cc: Mauro Carvalho Chehab, Arnd Bergmann, Gustavo A. R. Silva, Sakari Ailus, linux-kernel, linux-media On Thu, Jun 24, 2021 at 01:12:11AM +0300, Laurent Pinchart wrote: > Hi Kees, > > On Wed, Jun 16, 2021 at 09:22:23PM -0700, Kees Cook wrote: > > On Wed, Jun 16, 2021 at 10:43:03PM +0300, Laurent Pinchart wrote: > > > On Wed, Jun 16, 2021 at 11:59:38AM -0700, Kees Cook wrote: > > > > Avoid writing past the end of a structure member by wrapping the target > > > > region in a common named structure. This additionally fixes a > > > > misalignment of the copy (since the size of "buf" changes between 64-bit > > > > and 32-bit). > > > > > > Could you have been mislead by the data64 name ? The difference between > > > omap3isp_stat_data_time and omap3isp_stat_data_time32 is the size of the > > > ts field, using 32-bit timestamps with legacy userspace, and 64-bit > > > timestamps with more recent userspace. In both cases we're dealing with > > > a 32-bit platform, as the omap3isp is not used in any 64-bit ARM SoC. > > > The size of void __user *buf is thus 4 bytes in all cases, as is __u32 > > > buf. > > > > Ah, yes, that's true. I was hitting this on arm64 builds > > (CONFIG_COMPILE_TEST) where __user *buf is 64-bit. So, the "additionally > > fixes" bit above is misleading in the sense that nothing was ever built > > in the real world like that. > > > > The patch still fixes the compile-time warnings, though. > > I What's the compile-time warning ? I tried compiling the driver for > ARM64 and didn't notice any. Sorry, I didn't include the background well enough in the commit log, but it's part of a tightening of memcpy() under FORTIFY_SOURCE and also -Warray-bounds enablement. Here's what I've been saying on other patches (this one was different because it seemed to be just broken code): 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. Anyway, I can carry this until the full series is posted, but I'm still working through a few more fixes before I send the whole thing. This patch was one of a handful that didn't have any series dependencies. -Kees -- Kees Cook ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-06-23 22:53 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-16 18:59 [PATCH] media: omap3isp: Extract struct group for memcpy() region Kees Cook 2021-06-16 19:43 ` Laurent Pinchart 2021-06-17 4:22 ` Kees Cook 2021-06-23 22:12 ` Laurent Pinchart 2021-06-23 22:53 ` Kees Cook
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).