diff mbox series

[01/64] media: omap3isp: Extract struct group for memcpy() region

Message ID 20210727205855.411487-2-keescook@chromium.org (mailing list archive)
State New, archived
Headers show
Series Introduce strict memcpy() bounds checking | expand

Commit Message

Kees Cook July 27, 2021, 8:57 p.m. UTC
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 a common named structure. 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             | 44 +++++++++++++++++------
 2 files changed, 36 insertions(+), 13 deletions(-)

Comments

Gustavo A. R. Silva July 28, 2021, 12:55 a.m. UTC | #1
On Tue, Jul 27, 2021 at 01:57:52PM -0700, 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 a common named structure. 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             | 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));

I think this should be:

	memcpy(..., &data64.frame, ...);

instead.

--
Gustavo

>  
>  	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.30.2
>
Kees Cook July 28, 2021, 1:50 a.m. UTC | #2
On Tue, Jul 27, 2021 at 07:55:46PM -0500, Gustavo A. R. Silva wrote:
> On Tue, Jul 27, 2021 at 01:57:52PM -0700, 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 a common named structure. 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             | 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));
> 
> I think this should be:
> 
> 	memcpy(..., &data64.frame, ...);
> 
> instead.

Whoops; thanks! This is what I get for temporarily silencing the
read-overflow warnings. :)

-Kees
David Sterba July 28, 2021, 8:59 a.m. UTC | #3
On Tue, Jul 27, 2021 at 01:57:52PM -0700, 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 a common named structure. 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             | 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 = { };

Should this be { 0 } ?

We've seen patches trying to switch from { 0 } to {  } but the answer
was that { 0 } is supposed to be used,
http://www.ex-parrot.com/~chris/random/initialise.html

(from https://lore.kernel.org/lkml/fbddb15a-6e46-3f21-23ba-b18f66e3448a@suse.com/)
Dan Carpenter July 28, 2021, 9:14 a.m. UTC | #4
On Wed, Jul 28, 2021 at 10:59:22AM +0200, David Sterba wrote:
> >  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 = { };
> 
> Should this be { 0 } ?
> 
> We've seen patches trying to switch from { 0 } to {  } but the answer
> was that { 0 } is supposed to be used,
> http://www.ex-parrot.com/~chris/random/initialise.html
> 
> (from https://lore.kernel.org/lkml/fbddb15a-6e46-3f21-23ba-b18f66e3448a@suse.com/)

In the kernel we don't care about portability so much.  Use the = { }
GCC extension.  If the first member of the struct is a pointer then
Sparse will complain about = { 0 }.

I had a patch to make checkpatch.pl complain about = { 0 }; but my
system died and I haven't transfered my postponed messages to the new
system...

regards,
dan carpenter
Bart Van Assche July 28, 2021, 9:37 p.m. UTC | #5
On 7/28/21 2:14 AM, Dan Carpenter wrote:
> On Wed, Jul 28, 2021 at 10:59:22AM +0200, David Sterba wrote:
>>>   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 = { };
>>
>> Should this be { 0 } ?
>>
>> We've seen patches trying to switch from { 0 } to {  } but the answer
>> was that { 0 } is supposed to be used,
>> http://www.ex-parrot.com/~chris/random/initialise.html
>>
>> (from https://lore.kernel.org/lkml/fbddb15a-6e46-3f21-23ba-b18f66e3448a@suse.com/)
> 
> In the kernel we don't care about portability so much.  Use the = { }
> GCC extension.  If the first member of the struct is a pointer then
> Sparse will complain about = { 0 }.

+1 for { }. BTW, my understanding is that neither the C standard nor the 
C++ standard guarantee anything about initialization of padding bytes 
nor about the initialization of unnamed bitfields for stack variables 
when using aggregate initialization.

Bart.
David Sterba July 28, 2021, 9:37 p.m. UTC | #6
On Wed, Jul 28, 2021 at 02:37:20PM -0700, Bart Van Assche wrote:
> On 7/28/21 2:14 AM, Dan Carpenter wrote:
> > On Wed, Jul 28, 2021 at 10:59:22AM +0200, David Sterba wrote:
> >>>   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 = { };
> >>
> >> Should this be { 0 } ?
> >>
> >> We've seen patches trying to switch from { 0 } to {  } but the answer
> >> was that { 0 } is supposed to be used,
> >> http://www.ex-parrot.com/~chris/random/initialise.html
> >>
> >> (from https://lore.kernel.org/lkml/fbddb15a-6e46-3f21-23ba-b18f66e3448a@suse.com/)
> > 
> > In the kernel we don't care about portability so much.  Use the = { }
> > GCC extension.  If the first member of the struct is a pointer then
> > Sparse will complain about = { 0 }.
> 
> +1 for { }.

Oh, I thought the tendency is is to use { 0 } because that can also
intialize the compound members, by a "scalar 0" as it appears in the
code.
Greg Kroah-Hartman July 29, 2021, 5:56 a.m. UTC | #7
On Wed, Jul 28, 2021 at 11:37:30PM +0200, David Sterba wrote:
> On Wed, Jul 28, 2021 at 02:37:20PM -0700, Bart Van Assche wrote:
> > On 7/28/21 2:14 AM, Dan Carpenter wrote:
> > > On Wed, Jul 28, 2021 at 10:59:22AM +0200, David Sterba wrote:
> > >>>   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 = { };
> > >>
> > >> Should this be { 0 } ?
> > >>
> > >> We've seen patches trying to switch from { 0 } to {  } but the answer
> > >> was that { 0 } is supposed to be used,
> > >> http://www.ex-parrot.com/~chris/random/initialise.html
> > >>
> > >> (from https://lore.kernel.org/lkml/fbddb15a-6e46-3f21-23ba-b18f66e3448a@suse.com/)
> > > 
> > > In the kernel we don't care about portability so much.  Use the = { }
> > > GCC extension.  If the first member of the struct is a pointer then
> > > Sparse will complain about = { 0 }.
> > 
> > +1 for { }.
> 
> Oh, I thought the tendency is is to use { 0 } because that can also
> intialize the compound members, by a "scalar 0" as it appears in the
> code.
> 

Holes in the structure might not be initialized to anything if you do
either one of these as well.

Or did we finally prove that is not the case?  I can not remember
anymore...

greg k-h
Dan Carpenter July 29, 2021, 8:20 a.m. UTC | #8
On Thu, Jul 29, 2021 at 07:56:27AM +0200, Greg Kroah-Hartman wrote:
> On Wed, Jul 28, 2021 at 11:37:30PM +0200, David Sterba wrote:
> > On Wed, Jul 28, 2021 at 02:37:20PM -0700, Bart Van Assche wrote:
> > > On 7/28/21 2:14 AM, Dan Carpenter wrote:
> > > > On Wed, Jul 28, 2021 at 10:59:22AM +0200, David Sterba wrote:
> > > >>>   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 = { };
> > > >>
> > > >> Should this be { 0 } ?
> > > >>
> > > >> We've seen patches trying to switch from { 0 } to {  } but the answer
> > > >> was that { 0 } is supposed to be used,
> > > >> http://www.ex-parrot.com/~chris/random/initialise.html 
> > > >>
> > > >> (from https://lore.kernel.org/lkml/fbddb15a-6e46-3f21-23ba-b18f66e3448a@suse.com/ )
> > > > 
> > > > In the kernel we don't care about portability so much.  Use the = { }
> > > > GCC extension.  If the first member of the struct is a pointer then
> > > > Sparse will complain about = { 0 }.
> > > 
> > > +1 for { }.
> > 
> > Oh, I thought the tendency is is to use { 0 } because that can also
> > intialize the compound members, by a "scalar 0" as it appears in the
> > code.
> > 
> 
> Holes in the structure might not be initialized to anything if you do
> either one of these as well.
> 
> Or did we finally prove that is not the case?  I can not remember
> anymore...

Yep.  The C11 spec says that struct holes are initialized.

https://lore.kernel.org/netdev/20200731140452.GE24045@ziepe.ca/

What doesn't initialize struct holes is assignments:

	struct foo foo = *bar;

regards,
dan carpenter
Kees Cook July 30, 2021, 6 a.m. UTC | #9
On Thu, Jul 29, 2021 at 11:20:39AM +0300, Dan Carpenter wrote:
> On Thu, Jul 29, 2021 at 07:56:27AM +0200, Greg Kroah-Hartman wrote:
> > On Wed, Jul 28, 2021 at 11:37:30PM +0200, David Sterba wrote:
> > > On Wed, Jul 28, 2021 at 02:37:20PM -0700, Bart Van Assche wrote:
> > > > On 7/28/21 2:14 AM, Dan Carpenter wrote:
> > > > > On Wed, Jul 28, 2021 at 10:59:22AM +0200, David Sterba wrote:
> > > > >>>   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 = { };
> > > > >>
> > > > >> Should this be { 0 } ?
> > > > >>
> > > > >> We've seen patches trying to switch from { 0 } to {  } but the answer
> > > > >> was that { 0 } is supposed to be used,
> > > > >> http://www.ex-parrot.com/~chris/random/initialise.html 
> > > > >>
> > > > >> (from https://lore.kernel.org/lkml/fbddb15a-6e46-3f21-23ba-b18f66e3448a@suse.com/ )
> > > > > 
> > > > > In the kernel we don't care about portability so much.  Use the = { }
> > > > > GCC extension.  If the first member of the struct is a pointer then
> > > > > Sparse will complain about = { 0 }.
> > > > 
> > > > +1 for { }.
> > > 
> > > Oh, I thought the tendency is is to use { 0 } because that can also
> > > intialize the compound members, by a "scalar 0" as it appears in the
> > > code.
> > > 
> > 
> > Holes in the structure might not be initialized to anything if you do
> > either one of these as well.
> > 
> > Or did we finally prove that is not the case?  I can not remember
> > anymore...
> 
> Yep.  The C11 spec says that struct holes are initialized.
> 
> https://lore.kernel.org/netdev/20200731140452.GE24045@ziepe.ca/

This is, unfortunately, misleading. The frustrating key word is
"partial" in "updated in C11 to require zero'ing padding when doing
partial initialization of aggregates". If one initializes _all_ the
struct members ... the padding doesn't get initialized. :( (And until
recently, _trailing_ padding wasn't getting initialized even when other
paddings were.)

I've tried to collect all the different ways the compiler might initialize
a variable in this test:
https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/tree/lib/test_stackinit.c?h=for-next/kspp

FWIW, there's no difference between -std=gnu99 and -std=c11, and the
test shows that padding is _not_ universally initialized (unless your
compiler supports -ftrivial-auto-var-init=zero, which Clang does, and
GCC will shortly[1]). Running this with GCC 10.3.0, I see this...

As expected, having no initializer leaves padding (as well as members)
uninitialized:

stackinit: small_hole_none FAIL (uninit bytes: 24)
stackinit: big_hole_none FAIL (uninit bytes: 128)
stackinit: trailing_hole_none FAIL (uninit bytes: 32)

Here, "zero" means  "= { };" and they get padding initialized:

stackinit: small_hole_zero ok
stackinit: big_hole_zero ok
stackinit: trailing_hole_zero ok

Here, "static_partial" means "= { .one_member = 0 };", and
"dynamic_partial" means "= { .one_member = some_variable };". These are
similarly initialized:

stackinit: small_hole_static_partial ok
stackinit: big_hole_static_partial ok
stackinit: trailing_hole_static_partial ok

stackinit: small_hole_dynamic_partial ok
stackinit: big_hole_dynamic_partial ok
stackinit: trailing_hole_dynamic_partial ok

But when _all_ members are initialized, the padding is _not_:

stackinit: small_hole_static_all FAIL (uninit bytes: 3)
stackinit: big_hole_static_all FAIL (uninit bytes: 124)
stackinit: trailing_hole_static_all FAIL (uninit bytes: 7)

stackinit: small_hole_dynamic_all FAIL (uninit bytes: 3)
stackinit: big_hole_dynamic_all FAIL (uninit bytes: 124)
stackinit: trailing_hole_dynamic_all FAIL (uninit bytes: 7)

As expected, assigning to members outside of initialization leaves
padding uninitialized:

stackinit: small_hole_runtime_partial FAIL (uninit bytes: 23)
stackinit: big_hole_runtime_partial FAIL (uninit bytes: 127)
stackinit: trailing_hole_runtime_partial FAIL (uninit bytes: 24)

stackinit: small_hole_runtime_all FAIL (uninit bytes: 3)
stackinit: big_hole_runtime_all FAIL (uninit bytes: 124)
stackinit: trailing_hole_runtime_all FAIL (uninit bytes: 7)

> What doesn't initialize struct holes is assignments:
> 
> 	struct foo foo = *bar;

Right. Object to object assignments do not clear padding:

stackinit: small_hole_assigned_copy XFAIL (uninit bytes: 3)
stackinit: big_hole_assigned_copy XFAIL (uninit bytes: 124)
stackinit: trailing_hole_assigned_copy XFAIL (uninit bytes: 7)

And whole-object assignments of cast initializers follow the pattern of
basic initializers, which makes sense given the behavior of initializers
and direct assignment tests above. e.g.:
	obj = (type){ .member = ... };

stackinit: small_hole_assigned_static_partial ok
stackinit: small_hole_assigned_dynamic_partial ok
stackinit: big_hole_assigned_dynamic_partial ok
stackinit: big_hole_assigned_static_partial ok
stackinit: trailing_hole_assigned_dynamic_partial ok
stackinit: trailing_hole_assigned_static_partial ok

stackinit: small_hole_assigned_static_all FAIL (uninit bytes: 3)
stackinit: small_hole_assigned_dynamic_all FAIL (uninit bytes: 3)
stackinit: big_hole_assigned_static_all FAIL (uninit bytes: 124)
stackinit: big_hole_assigned_dynamic_all FAIL (uninit bytes: 124)
stackinit: trailing_hole_assigned_dynamic_all FAIL (uninit bytes: 7)
stackinit: trailing_hole_assigned_static_all FAIL (uninit bytes: 7)

So, yeah, it's not very stable.

-Kees

[1] https://gcc.gnu.org/pipermail/gcc-patches/2021-July/576341.html
David Sterba July 30, 2021, 8:38 a.m. UTC | #10
On Thu, Jul 29, 2021 at 11:00:48PM -0700, Kees Cook wrote:
> On Thu, Jul 29, 2021 at 11:20:39AM +0300, Dan Carpenter wrote:
> > On Thu, Jul 29, 2021 at 07:56:27AM +0200, Greg Kroah-Hartman wrote:
> > > On Wed, Jul 28, 2021 at 11:37:30PM +0200, David Sterba wrote:
> > > > On Wed, Jul 28, 2021 at 02:37:20PM -0700, Bart Van Assche wrote:
> > > > > On 7/28/21 2:14 AM, Dan Carpenter wrote:
> > > > > > On Wed, Jul 28, 2021 at 10:59:22AM +0200, David Sterba wrote:
> > > > > >>>   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 = { };
> > > > > >>
> > > > > >> Should this be { 0 } ?
> > > > > >>
> > > > > >> We've seen patches trying to switch from { 0 } to {  } but the answer
> > > > > >> was that { 0 } is supposed to be used,
> > > > > >> http://www.ex-parrot.com/~chris/random/initialise.html 
> > > > > >>
> > > > > >> (from https://lore.kernel.org/lkml/fbddb15a-6e46-3f21-23ba-b18f66e3448a@suse.com/ )
> > > > > > 
> > > > > > In the kernel we don't care about portability so much.  Use the = { }
> > > > > > GCC extension.  If the first member of the struct is a pointer then
> > > > > > Sparse will complain about = { 0 }.
> > > > > 
> > > > > +1 for { }.
> > > > 
> > > > Oh, I thought the tendency is is to use { 0 } because that can also
> > > > intialize the compound members, by a "scalar 0" as it appears in the
> > > > code.
> > > > 
> > > 
> > > Holes in the structure might not be initialized to anything if you do
> > > either one of these as well.
> > > 
> > > Or did we finally prove that is not the case?  I can not remember
> > > anymore...
> > 
> > Yep.  The C11 spec says that struct holes are initialized.
> > 
> > https://lore.kernel.org/netdev/20200731140452.GE24045@ziepe.ca/
> 
> This is, unfortunately, misleading. The frustrating key word is
> "partial" in "updated in C11 to require zero'ing padding when doing
> partial initialization of aggregates". If one initializes _all_ the
> struct members ... the padding doesn't get initialized. :( (And until
> recently, _trailing_ padding wasn't getting initialized even when other
> paddings were.)
> 
> I've tried to collect all the different ways the compiler might initialize
> a variable in this test:
> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/tree/lib/test_stackinit.c?h=for-next/kspp
> 
> FWIW, there's no difference between -std=gnu99 and -std=c11, and the
> test shows that padding is _not_ universally initialized (unless your
> compiler supports -ftrivial-auto-var-init=zero, which Clang does, and
> GCC will shortly[1]). Running this with GCC 10.3.0, I see this...
> 
> As expected, having no initializer leaves padding (as well as members)
> uninitialized:
> 
> stackinit: small_hole_none FAIL (uninit bytes: 24)
> stackinit: big_hole_none FAIL (uninit bytes: 128)
> stackinit: trailing_hole_none FAIL (uninit bytes: 32)
> 
> Here, "zero" means  "= { };" and they get padding initialized:
> 
> stackinit: small_hole_zero ok
> stackinit: big_hole_zero ok
> stackinit: trailing_hole_zero ok
> 
> Here, "static_partial" means "= { .one_member = 0 };", and
> "dynamic_partial" means "= { .one_member = some_variable };". These are
> similarly initialized:
> 
> stackinit: small_hole_static_partial ok
> stackinit: big_hole_static_partial ok
> stackinit: trailing_hole_static_partial ok
> 
> stackinit: small_hole_dynamic_partial ok
> stackinit: big_hole_dynamic_partial ok
> stackinit: trailing_hole_dynamic_partial ok
> 
> But when _all_ members are initialized, the padding is _not_:
> 
> stackinit: small_hole_static_all FAIL (uninit bytes: 3)
> stackinit: big_hole_static_all FAIL (uninit bytes: 124)
> stackinit: trailing_hole_static_all FAIL (uninit bytes: 7)
> 
> stackinit: small_hole_dynamic_all FAIL (uninit bytes: 3)
> stackinit: big_hole_dynamic_all FAIL (uninit bytes: 124)
> stackinit: trailing_hole_dynamic_all FAIL (uninit bytes: 7)
> 
> As expected, assigning to members outside of initialization leaves
> padding uninitialized:
> 
> stackinit: small_hole_runtime_partial FAIL (uninit bytes: 23)
> stackinit: big_hole_runtime_partial FAIL (uninit bytes: 127)
> stackinit: trailing_hole_runtime_partial FAIL (uninit bytes: 24)
> 
> stackinit: small_hole_runtime_all FAIL (uninit bytes: 3)
> stackinit: big_hole_runtime_all FAIL (uninit bytes: 124)
> stackinit: trailing_hole_runtime_all FAIL (uninit bytes: 7)
> 
> > What doesn't initialize struct holes is assignments:
> > 
> > 	struct foo foo = *bar;
> 
> Right. Object to object assignments do not clear padding:
> 
> stackinit: small_hole_assigned_copy XFAIL (uninit bytes: 3)
> stackinit: big_hole_assigned_copy XFAIL (uninit bytes: 124)
> stackinit: trailing_hole_assigned_copy XFAIL (uninit bytes: 7)
> 
> And whole-object assignments of cast initializers follow the pattern of
> basic initializers, which makes sense given the behavior of initializers
> and direct assignment tests above. e.g.:
> 	obj = (type){ .member = ... };
> 
> stackinit: small_hole_assigned_static_partial ok
> stackinit: small_hole_assigned_dynamic_partial ok
> stackinit: big_hole_assigned_dynamic_partial ok
> stackinit: big_hole_assigned_static_partial ok
> stackinit: trailing_hole_assigned_dynamic_partial ok
> stackinit: trailing_hole_assigned_static_partial ok
> 
> stackinit: small_hole_assigned_static_all FAIL (uninit bytes: 3)
> stackinit: small_hole_assigned_dynamic_all FAIL (uninit bytes: 3)
> stackinit: big_hole_assigned_static_all FAIL (uninit bytes: 124)
> stackinit: big_hole_assigned_dynamic_all FAIL (uninit bytes: 124)
> stackinit: trailing_hole_assigned_dynamic_all FAIL (uninit bytes: 7)
> stackinit: trailing_hole_assigned_static_all FAIL (uninit bytes: 7)
> 
> So, yeah, it's not very stable.

Then is explicit memset the only reliable way accross all compiler
flavors and supported versions?

E.g. for ioctls that get kernel memory (stack, kmalloc), partially
initialize it and then call copy_to_user.
Dan Carpenter July 30, 2021, 9 a.m. UTC | #11
On Fri, Jul 30, 2021 at 10:38:45AM +0200, David Sterba wrote:
> Then is explicit memset the only reliable way accross all compiler
> flavors and supported versions?
> 

The = { } initializer works.  It's only when you start partially
initializing the struct that it doesn't initialize holes.

regards,
dan carpenter
Kees Cook July 30, 2021, 4:44 p.m. UTC | #12
On Fri, Jul 30, 2021 at 12:00:54PM +0300, Dan Carpenter wrote:
> On Fri, Jul 30, 2021 at 10:38:45AM +0200, David Sterba wrote:
> > Then is explicit memset the only reliable way accross all compiler
> > flavors and supported versions?
> > 
> 
> The = { } initializer works.  It's only when you start partially
> initializing the struct that it doesn't initialize holes.

No, partial works. It's when you _fully_ initialize the struct where the
padding doesn't get initialized. *sob*

struct foo {
	u8 flag;
	/* padding */
	void *ptr;
};

These are fine:

struct foo ok1 = { };
struct foo ok2 = { .flag = 7 };
struct foo ok3 = { .ptr = NULL };

This is not:

struct foo bad = { .flag = 7, .ptr = NULL };

(But, of course, it depends on padding size, compiler version, and
architecture. i.e. things remain unreliable.)
Nick Desaulniers July 30, 2021, 5:08 p.m. UTC | #13
On Fri, Jul 30, 2021 at 9:44 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Fri, Jul 30, 2021 at 12:00:54PM +0300, Dan Carpenter wrote:
> > On Fri, Jul 30, 2021 at 10:38:45AM +0200, David Sterba wrote:
> > > Then is explicit memset the only reliable way accross all compiler
> > > flavors and supported versions?
> > >
> >
> > The = { } initializer works.  It's only when you start partially
> > initializing the struct that it doesn't initialize holes.
>
> No, partial works. It's when you _fully_ initialize the struct where the
> padding doesn't get initialized. *sob*

I'm pretty sure that this has more to do with whether or not the
compiler applies SROA then observes uses of the individual members or
not.

>
> struct foo {
>         u8 flag;
>         /* padding */
>         void *ptr;
> };
>
> These are fine:
>
> struct foo ok1 = { };
> struct foo ok2 = { .flag = 7 };
> struct foo ok3 = { .ptr = NULL };
>
> This is not:
>
> struct foo bad = { .flag = 7, .ptr = NULL };
>
> (But, of course, it depends on padding size, compiler version, and
> architecture. i.e. things remain unreliable.)
>
> --
Kees Cook July 30, 2021, 7:18 p.m. UTC | #14
On Fri, Jul 30, 2021 at 10:08:03AM -0700, Nick Desaulniers wrote:
> On Fri, Jul 30, 2021 at 9:44 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Fri, Jul 30, 2021 at 12:00:54PM +0300, Dan Carpenter wrote:
> > > On Fri, Jul 30, 2021 at 10:38:45AM +0200, David Sterba wrote:
> > > > Then is explicit memset the only reliable way accross all compiler
> > > > flavors and supported versions?
> > > >
> > >
> > > The = { } initializer works.  It's only when you start partially
> > > initializing the struct that it doesn't initialize holes.
> >
> > No, partial works. It's when you _fully_ initialize the struct where the
> > padding doesn't get initialized. *sob*
> 
> I'm pretty sure that this has more to do with whether or not the
> compiler applies SROA then observes uses of the individual members or
> not.

Ultimately, it's just not consistent, so thank goodness for
-ftrivial-auto-var-init=zero. :)
diff mbox series

Patch

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