Introduce strict memcpy() bounds checking

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 <[email protected]>

---
 drivers/media/platform/omap3isp/ispstat.c |  5 +--
 include/uapi/linux/omap3isp.h             | 44 +++++++++++++++++------
 2 files changed, 36 insertions(+), 13 deletions(-)

drivers/media/platform/omap3isp/ispstat.c +1 -1

index 5b9b57f4d9bf..ea8222fed38e 100644
@@ -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);

drivers/media/platform/omap3isp/ispstat.c +0 -0

 	return 0;
 }

include/uapi/linux/omap3isp.h +33 -11

index 87b55755f4ff..0a16af91621f 100644
@@ -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
 

Signature · 1 line

2.30.2

Quoted 59 lines · Kees Cook

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 <[email protected]>

---
 drivers/media/platform/omap3isp/ispstat.c |  5 +--
 include/uapi/linux/omap3isp.h             | 44 +++++++++++++++++------
 2 files changed, 36 insertions(+), 13 deletions(-)

drivers/media/platform/omap3isp/ispstat.c +1 -1

index 5b9b57f4d9bf..ea8222fed38e 100644
@@ -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);

drivers/media/platform/omap3isp/ispstat.c int omap3isp_stat_request_statistics_time32(struct ispstat *stat, · line 525

Quoted 6 lines

drivers/media/platform/omap3isp/ispstat.c +2 -1

@@ -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.

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 <[email protected]>

---
 drivers/media/platform/omap3isp/ispstat.c |  5 +--
 include/uapi/linux/omap3isp.h             | 44 +++++++++++++++++------
 2 files changed, 36 insertions(+), 13 deletions(-)

drivers/media/platform/omap3isp/ispstat.c +0 -0

index 5b9b57f4d9bf..ea8222fed38e 100644

Quoted 9 lines

 drivers/media/platform/omap3isp/ispstat.c |  5 +--
 include/uapi/linux/omap3isp.h             | 44 +++++++++++++++++------
 2 files changed, 36 insertions(+), 13 deletions(-)

drivers/media/platform/omap3isp/ispstat.c +0 -0

index 5b9b57f4d9bf..ea8222fed38e 100644

drivers/media/platform/omap3isp/ispstat.c int omap3isp_stat_request_statistics(struct ispstat *stat, · line 515

Quoted 5 lines

drivers/media/platform/omap3isp/ispstat.c +1 -1

@@ -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/[email protected]/)

Quoted 21 lines · David Sterba

Quoted 9 lines

  drivers/media/platform/omap3isp/ispstat.c |  5 +--
  include/uapi/linux/omap3isp.h             | 44 +++++++++++++++++------
  2 files changed, 36 insertions(+), 13 deletions(-)

drivers/media/platform/omap3isp/ispstat.c +0 -0

index 5b9b57f4d9bf..ea8222fed38e 100644

drivers/media/platform/omap3isp/ispstat.c int omap3isp_stat_request_statistics(struct ispstat *stat, · line 515

Quoted 5 lines

drivers/media/platform/omap3isp/ispstat.c +1 -1

@@ -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

Quoted 1 line

(from https://lore.kernel.org/lkml/[email protected]/)

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 }.

Quoted 22 lines · Dan Carpenter

Quoted 21 lines · David Sterba

Quoted 9 lines

  drivers/media/platform/omap3isp/ispstat.c |  5 +--
  include/uapi/linux/omap3isp.h             | 44 +++++++++++++++++------
  2 files changed, 36 insertions(+), 13 deletions(-)

drivers/media/platform/omap3isp/ispstat.c +0 -0

index 5b9b57f4d9bf..ea8222fed38e 100644

drivers/media/platform/omap3isp/ispstat.c int omap3isp_stat_request_statistics(struct ispstat *stat, · line 515

Quoted 5 lines

drivers/media/platform/omap3isp/ispstat.c +1 -1

@@ -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

Quoted 5 lines

Quoted 1 line

(from https://lore.kernel.org/lkml/[email protected]/)

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 { }.

Quoted 29 lines · Bart Van Assche

Quoted 27 lines · Dan Carpenter

Quoted 17 lines · David Sterba

Quoted 9 lines

  drivers/media/platform/omap3isp/ispstat.c |  5 +--
  include/uapi/linux/omap3isp.h             | 44 +++++++++++++++++------
  2 files changed, 36 insertions(+), 13 deletions(-)

drivers/media/platform/omap3isp/ispstat.c +0 -0

index 5b9b57f4d9bf..ea8222fed38e 100644

drivers/media/platform/omap3isp/ispstat.c int omap3isp_stat_request_statistics(struct ispstat *stat, · line 515

Quoted 5 lines

drivers/media/platform/omap3isp/ispstat.c +1 -1

@@ -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 } ?

Quoted 5 lines

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/[email protected]/)

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 }.

Quoted 1 line

+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.

Quoted 30 lines · David Sterba

Quoted 29 lines · Bart Van Assche

Quoted 27 lines · Dan Carpenter

Quoted 17 lines · David Sterba

Quoted 9 lines

  drivers/media/platform/omap3isp/ispstat.c |  5 +--
  include/uapi/linux/omap3isp.h             | 44 +++++++++++++++++------
  2 files changed, 36 insertions(+), 13 deletions(-)

drivers/media/platform/omap3isp/ispstat.c +0 -0

index 5b9b57f4d9bf..ea8222fed38e 100644

drivers/media/platform/omap3isp/ispstat.c int omap3isp_stat_request_statistics(struct ispstat *stat, · line 515

Quoted 5 lines

drivers/media/platform/omap3isp/ispstat.c +1 -1

@@ -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 } ?

Quoted 5 lines

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/[email protected]/ )

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 }.

Quoted 6 lines

Quoted 1 line

+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...

Quoted 41 lines · Greg Kroah-Hartman

Quoted 30 lines · David Sterba

Quoted 29 lines · Bart Van Assche

Quoted 27 lines · Dan Carpenter

Quoted 17 lines · David Sterba

Quoted 9 lines

  drivers/media/platform/omap3isp/ispstat.c |  5 +--
  include/uapi/linux/omap3isp.h             | 44 +++++++++++++++++------
  2 files changed, 36 insertions(+), 13 deletions(-)

drivers/media/platform/omap3isp/ispstat.c +0 -0

index 5b9b57f4d9bf..ea8222fed38e 100644

drivers/media/platform/omap3isp/ispstat.c int omap3isp_stat_request_statistics(struct ispstat *stat, · line 515

Quoted 5 lines

drivers/media/platform/omap3isp/ispstat.c +1 -1

@@ -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 } ?

Quoted 5 lines

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/[email protected]/ )

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 }.

Quoted 6 lines

Quoted 1 line

+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.

Quoted 2 lines

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/[email protected]/

What doesn't initialize struct holes is assignments:

	struct foo foo = *bar;

Quoted 42 lines · Dan Carpenter

Quoted 41 lines · Greg Kroah-Hartman

Quoted 30 lines · David Sterba

Quoted 29 lines · Bart Van Assche

Quoted 27 lines · Dan Carpenter

Quoted 17 lines · David Sterba

Quoted 9 lines

  drivers/media/platform/omap3isp/ispstat.c |  5 +--
  include/uapi/linux/omap3isp.h             | 44 +++++++++++++++++------
  2 files changed, 36 insertions(+), 13 deletions(-)

drivers/media/platform/omap3isp/ispstat.c +0 -0

index 5b9b57f4d9bf..ea8222fed38e 100644

drivers/media/platform/omap3isp/ispstat.c int omap3isp_stat_request_statistics(struct ispstat *stat, · line 515

Quoted 5 lines

drivers/media/platform/omap3isp/ispstat.c +1 -1

@@ -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 } ?

Quoted 5 lines

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/[email protected]/ )

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 }.

Quoted 6 lines

Quoted 1 line

+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.

Quoted 6 lines

Quoted 2 lines

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/[email protected]/

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)

Quoted 3 lines

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?

Quoted 3 lines · David Sterba

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.

Quoted 5 lines · Dan Carpenter

Quoted 3 lines · David Sterba

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

Quoted 2 lines

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.)

--

Quoted 10 lines · Kees Cook

Quoted 7 lines · Dan Carpenter

Quoted 3 lines · David Sterba

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.

Quoted 2 lines

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.

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.

The it_present member of struct ieee80211_radiotap_header is treated as a flexible array (multiple u32s can be conditionally present). In order for memcpy() to reason (or really, not reason) about the size of operations against this struct, use of bytes beyond it_present need to be treated as part of the flexible array. Add a union/struct to contain the new "bitmap" member, for use with trailing presence bitmaps and arguments.

Additionally improve readability in the iterator code which walks through the bitmaps and arguments.

Signed-off-by Kees Cook <[email protected]>

---
 include/net/ieee80211_radiotap.h | 24 ++++++++++++++++++++----
 net/mac80211/rx.c                |  2 +-
 net/wireless/radiotap.c          |  5 ++---
 3 files changed, 23 insertions(+), 8 deletions(-)

include/net/ieee80211_radiotap.h +11 -4

index c0854933e24f..101c1e961032 100644
@@ -39,10 +39,26 @@ struct ieee80211_radiotap_header {
 	 */
 	__le16 it_len;
 
-	/**
-	 * @it_present: (first) present word
-	 */
-	__le32 it_present;
+	union {
+		/**
+		 * @it_present: (first) present word
+		 */
+		__le32 it_present;
+
+		struct {
+			/* The compiler makes it difficult to overlap
+			 * a flex-array with an existing singleton,
+			 * so we're forced to add an empty named
+			 * variable here.

net/mac80211/rx.c ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, · line 375

Quoted 6 lines · Dan Carpenter

net/mac80211/rx.c +1 -1

@@ -372,7 +372,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local,
 			ieee80211_calculate_rx_timestamp(local, status,
 							 mpdulen, 0),
 			pos);
-		rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_TSFT);
+		rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_TSFT);

A drive-by comment, not related to the patchset, but rather the ieee80211 driver itself.

Shift expressions with (1 << NUMBER) can be subtly broken once the NUMBER is 31 and the value gets silently cast to a 64bit type. It will become 0xfffffffff80000000.

I've checked the IEEE80211_RADIOTAP_* defintions if this is even remotely possible and yes, IEEE80211_RADIOTAP_EXT == 31. Fortunatelly it seems to be used with used with a 32bit types (eg. _bitmap_shifter) so there are no surprises.

The recommended practice is to always use unsigned types for shifts, so "1U << ..." at least.

Quoted 20 lines · David Sterba

net/mac80211/rx.c ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, · line 375

Quoted 6 lines · Dan Carpenter

net/mac80211/rx.c +1 -1

@@ -372,7 +372,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local,
 			ieee80211_calculate_rx_timestamp(local, status,
 							 mpdulen, 0),
 			pos);
-		rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_TSFT);
+		rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_TSFT);

A drive-by comment, not related to the patchset, but rather the ieee80211 driver itself.

Shift expressions with (1 << NUMBER) can be subtly broken once the NUMBER is 31 and the value gets silently cast to a 64bit type. It will become 0xfffffffff80000000.

I've checked the IEEE80211_RADIOTAP_* defintions if this is even remotely possible and yes, IEEE80211_RADIOTAP_EXT == 31. Fortunatelly it seems to be used with used with a 32bit types (eg. _bitmap_shifter) so there are no surprises.

Quoted 2 lines

The recommended practice is to always use unsigned types for shifts, so "1U << ..." at least.

Ah, good catch! I think just using BIT() is the right replacement here, yes? I suppose that should be a separate patch.

Quoted 24 lines · Kees Cook

Quoted 22 lines · David Sterba

net/mac80211/rx.c ieee80211_add_rx_radiotap_header(struct ieee80211_local *local, · line 375

Quoted 6 lines · Dan Carpenter

net/mac80211/rx.c +1 -1

@@ -372,7 +372,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local,
 			ieee80211_calculate_rx_timestamp(local, status,
 							 mpdulen, 0),
 			pos);
-		rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_TSFT);
+		rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_TSFT);

A drive-by comment, not related to the patchset, but rather the ieee80211 driver itself.

Shift expressions with (1 << NUMBER) can be subtly broken once the NUMBER is 31 and the value gets silently cast to a 64bit type. It will become 0xfffffffff80000000.

I've checked the IEEE80211_RADIOTAP_* defintions if this is even remotely possible and yes, IEEE80211_RADIOTAP_EXT == 31. Fortunatelly it seems to be used with used with a 32bit types (eg. _bitmap_shifter) so there are no surprises.

The recommended practice is to always use unsigned types for shifts, so "1U << ..." at least.

Quoted 2 lines

Ah, good catch! I think just using BIT() is the right replacement here, yes? I suppose that should be a separate patch.

I found definition of BIT in vdso/bits.h, that does not sound like a standard header, besides that it shifts 1UL, that may not be necessary everywhere. IIRC there were objections against using the macro at all.

Quoted 48 lines · Kees Cook

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.

The it_present member of struct ieee80211_radiotap_header is treated as a flexible array (multiple u32s can be conditionally present). In order for memcpy() to reason (or really, not reason) about the size of operations against this struct, use of bytes beyond it_present need to be treated as part of the flexible array. Add a union/struct to contain the new "bitmap" member, for use with trailing presence bitmaps and arguments.

Additionally improve readability in the iterator code which walks through the bitmaps and arguments.

Signed-off-by Kees Cook <[email protected]>

---
 include/net/ieee80211_radiotap.h | 24 ++++++++++++++++++++----
 net/mac80211/rx.c                |  2 +-
 net/wireless/radiotap.c          |  5 ++---
 3 files changed, 23 insertions(+), 8 deletions(-)

include/net/ieee80211_radiotap.h +15 -4

index c0854933e24f..101c1e961032 100644
@@ -39,10 +39,26 @@ struct ieee80211_radiotap_header {
 	 */
 	__le16 it_len;
 
-	/**
-	 * @it_present: (first) present word
-	 */
-	__le32 it_present;
+	union {
+		/**
+		 * @it_present: (first) present word
+		 */
+		__le32 it_present;
+
+		struct {
+			/* The compiler makes it difficult to overlap
+			 * a flex-array with an existing singleton,
+			 * so we're forced to add an empty named
+			 * variable here.
+			 */
+			struct { } __unused;
+
+			/**

include/net/ieee80211_radiotap.h struct ieee80211_radiotap_header { · line 62

Quoted 6 lines

include/net/ieee80211_radiotap.h +5 -0

+			 * @bitmap: all presence bitmaps
+			 */
+			__le32 bitmap[];
+		};
+	};
 } __packed;

This patch is so confusing...

Btw, after the end of the __le32 data there is a bunch of other le64, u8 and le16 data so the struct is not accurate or complete.

It might be better to re-write this as something like this:

include/net/ieee80211_radiotap.h +0 -0

index c0854933e24f..0cb5719e9668 100644

net/mac80211/rx.c +11 -11

 	/* radiotap header, set always present flags */
 	rthdr->it_len = cpu_to_le16(rtap_len);

@@ -372,7 +372,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local,
 			ieee80211_calculate_rx_timestamp(local, status,
 							 mpdulen, 0),
 			pos);
-		rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_TSFT);
+		rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_TSFT);
 		pos += 8;
 	}
 

@@ -396,7 +396,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local,
 		*pos = 0;
 	} else {
 		int shift = 0;
-		rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_RATE);
+		rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_RATE);
 		if (status->bw == RATE_INFO_BW_10)
 			shift = 1;
 		else if (status->bw == RATE_INFO_BW_5)

@@ -432,7 +432,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local,
 	if (ieee80211_hw_check(&local->hw, SIGNAL_DBM) &&
 	    !(status->flag & RX_FLAG_NO_SIGNAL_VAL)) {
 		*pos = status->signal;
-		rthdr->it_present |=
+		rthdr->data.it_present |=
 			cpu_to_le32(1 << IEEE80211_RADIOTAP_DBM_ANTSIGNAL);
 		pos++;
 	}

@@ -459,7 +459,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local,
 	if (status->encoding == RX_ENC_HT) {
 		unsigned int stbc;
 
-		rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_MCS);
+		rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_MCS);
 		*pos++ = local->hw.radiotap_mcs_details;
 		*pos = 0;
 		if (status->enc_flags & RX_ENC_FLAG_SHORT_GI)

@@ -482,7 +482,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local,
 		/* ensure 4 byte alignment */
 		while ((pos - (u8 *)rthdr) & 3)
 			pos++;
-		rthdr->it_present |=
+		rthdr->data.it_present |=
 			cpu_to_le32(1 << IEEE80211_RADIOTAP_AMPDU_STATUS);
 		put_unaligned_le32(status->ampdu_reference, pos);
 		pos += 4;

@@ -510,7 +510,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local,
 	if (status->encoding == RX_ENC_VHT) {
 		u16 known = local->hw.radiotap_vht_details;
 
-		rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_VHT);
+		rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_VHT);
 		put_unaligned_le16(known, pos);
 		pos += 2;
 		/* flags */

@@ -553,7 +553,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local,
 		u16 accuracy = 0;
 		u8 flags = IEEE80211_RADIOTAP_TIMESTAMP_FLAG_32BIT;
 
-		rthdr->it_present |=
+		rthdr->data.it_present |=
 			cpu_to_le32(1 << IEEE80211_RADIOTAP_TIMESTAMP);
 
 		/* ensure 8 byte alignment */

@@ -642,7 +642,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local,
 		/* ensure 2 byte alignment */
 		while ((pos - (u8 *)rthdr) & 1)
 			pos++;
-		rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_HE);
+		rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_HE);
 		memcpy(pos, &he, sizeof(he));
 		pos += sizeof(he);
 	}

@@ -652,13 +652,13 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local,
 		/* ensure 2 byte alignment */
 		while ((pos - (u8 *)rthdr) & 1)
 			pos++;
-		rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_HE_MU);
+		rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_HE_MU);
 		memcpy(pos, &he_mu, sizeof(he_mu));
 		pos += sizeof(he_mu);
 	}
 
 	if (status->flag & RX_FLAG_NO_PSDU) {
-		rthdr->it_present |=
+		rthdr->data.it_present |=
 			cpu_to_le32(1 << IEEE80211_RADIOTAP_ZERO_LEN_PSDU);
 		*pos++ = status->zero_length_psdu_type;
 	}

@@ -667,7 +667,7 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local,
 		/* ensure 2 byte alignment */
 		while ((pos - (u8 *)rthdr) & 1)
 			pos++;
-		rthdr->it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_LSIG);
+		rthdr->data.it_present |= cpu_to_le32(1 << IEEE80211_RADIOTAP_LSIG);
 		memcpy(pos, &lsig, sizeof(lsig));
 		pos += sizeof(lsig);
 	}

net/wireless/radiotap.c +2 -3

index 36f1b59a78bf..f7852024c011 100644
@@ -114,11 +114,10 @@ int ieee80211_radiotap_iterator_init(
 	iterator->_rtheader = radiotap_header;
 	iterator->_max_length = get_unaligned_le16(&radiotap_header->it_len);
 	iterator->_arg_index = 0;
-	iterator->_bitmap_shifter = get_unaligned_le32(&radiotap_header->it_present);
+	iterator->_bitmap_shifter = get_unaligned_le32(&radiotap_header->data.it_present);
 	iterator->_arg = (uint8_t *)radiotap_header + sizeof(*radiotap_header);
 	iterator->_reset_on_ext = 0;
-	iterator->_next_bitmap = &radiotap_header->it_present;
-	iterator->_next_bitmap++;
+	iterator->_next_bitmap = (__le32 *)&radiotap_header->data.buff;
 	iterator->_vns = vns;
 	iterator->current_namespace = &radiotap_ns;
 	iterator->is_radiotap_ns = 1;

Quoted 48 lines · Kees Cook

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.

The it_present member of struct ieee80211_radiotap_header is treated as a flexible array (multiple u32s can be conditionally present). In order for memcpy() to reason (or really, not reason) about the size of operations against this struct, use of bytes beyond it_present need to be treated as part of the flexible array. Add a union/struct to contain the new "bitmap" member, for use with trailing presence bitmaps and arguments.

Additionally improve readability in the iterator code which walks through the bitmaps and arguments.

Signed-off-by Kees Cook <[email protected]>

---
 include/net/ieee80211_radiotap.h | 24 ++++++++++++++++++++----
 net/mac80211/rx.c                |  2 +-
 net/wireless/radiotap.c          |  5 ++---
 3 files changed, 23 insertions(+), 8 deletions(-)

include/net/ieee80211_radiotap.h +15 -4

index c0854933e24f..101c1e961032 100644
@@ -39,10 +39,26 @@ struct ieee80211_radiotap_header {
 	 */
 	__le16 it_len;
 
-	/**
-	 * @it_present: (first) present word
-	 */
-	__le32 it_present;
+	union {
+		/**
+		 * @it_present: (first) present word
+		 */
+		__le32 it_present;
+
+		struct {
+			/* The compiler makes it difficult to overlap
+			 * a flex-array with an existing singleton,
+			 * so we're forced to add an empty named
+			 * variable here.
+			 */
+			struct { } __unused;
+
+			/**

include/net/ieee80211_radiotap.h struct ieee80211_radiotap_header { · line 62

Quoted 6 lines

include/net/ieee80211_radiotap.h +5 -0

+			 * @bitmap: all presence bitmaps
+			 */
+			__le32 bitmap[];
+		};
+	};
 } __packed;

This patch is so confusing...

Btw, after the end of the __le32 data there is a bunch of other le64, u8 and le16 data so the struct is not accurate or complete.

Quoted 16 lines · Kees Cook

[...]

(quoted diff) +11 -4

-	/**
-	 * @it_present: (first) present word
-	 */
-	__le32 it_present;
+	union {
+		/**
+		 * @it_present: (first) present word
+		 */
+		__le32 it_present;
+
+		struct {
+			/* The compiler makes it difficult to overlap
+			 * a flex-array with an existing singleton,
+			 * so we're forced to add an empty named
+			 * variable here.

Review

Quoted 10 lines

(quoted diff) +9 -0

+			 */
+			struct { } __unused;
+
+			/**
+			 * @bitmap: all presence bitmaps
+			 */
+			__le32 bitmap[];
+		};
+	};
 } __packed;

This patch is so confusing...

Btw, after the end of the __le32 data there is a bunch of other le64, u8 and le16 data so the struct is not accurate or complete.

It might be better to re-write this as something like this:

include/net/ieee80211_radiotap.h +0 -0

index c0854933e24f..0cb5719e9668 100644

Ah-ha, got it:

The use of strncpy() is considered deprecated for NUL-terminated strings[1]. Replace strncpy() with strscpy_pad() (as it seems this case expects the NUL padding to fill the allocation following the flexible array). This additionally silences a warning seen when building under -Warray-bounds:

./include/linux/fortify-string.h:38:30: warning: '__builtin_strncpy' offset 24 from the object at '__mptr' is out of the bounds of referenced subobject 'data' with type 'u8[]' {aka 'unsigned char[]'} at offset 24 [-Warray-bounds]
38 | #define __underlying_strncpy __builtin_strncpy
| ^
./include/linux/fortify-string.h:50:9: note: in expansion of macro '__underlying_strncpy'

   50 |  return __underlying_strncpy(p, q, size);
      |         ^~~~~~~~~~~~~~~~~~~~
drivers/rpmsg/qcom_glink_native.c: In function 'qcom_glink_work':
drivers/rpmsg/qcom_glink_native.c:36:5: note: subobject 'data' declared here
   36 |  u8 data[];
      |     ^~~~

[1] https://www.kernel.org/doc/html/latest/process/deprecated.html#strncpy-on-nul-terminated-strings

Signed-off-by Kees Cook <[email protected]>

---
 drivers/rpmsg/qcom_glink_native.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/rpmsg/qcom_glink_native.c +1 -1

index 05533c71b10e..c7b9de655080 100644
@@ -1440,7 +1440,7 @@ static int qcom_glink_rx_open(struct qcom_glink *glink, unsigned int rcid,
 		}
 
 		rpdev->ept = &channel->ept;
-		strncpy(rpdev->id.name, name, RPMSG_NAME_SIZE);
+		strscpy_pad(rpdev->id.name, name, RPMSG_NAME_SIZE);
 		rpdev->src = RPMSG_ADDR_ANY;
 		rpdev->dst = RPMSG_ADDR_ANY;
 		rpdev->ops = &glink_device_ops;

Signature · 1 line

2.30.2

Kernel code has a regular need to describe groups of members within a structure usually when they need to be copied or initialized separately from the rest of the surrounding structure. The generally accepted design pattern in C is to use a named sub-struct:

	struct foo {
		int one;
		struct {
			int two;
			int three;
		} thing;
		int four;
	};

This would allow for traditional references and sizing:

	memcpy(&dst.thing, &src.thing, sizeof(dst.thing));

However, doing this would mean that referencing struct members enclosed by such named structs would always require including the sub-struct name in identifiers:

	do_something(dst.thing.three);

This has tended to be quite inflexible, especially when such groupings need to be added to established code which causes huge naming churn. Three workarounds exist in the kernel for this problem, and each have other negative properties.

To avoid the naming churn, there is a design pattern of adding macro aliases for the named struct:

	#define f_three thing.three

This ends up polluting the global namespace, and makes it difficult to search for identifiers.

Another common work-around in kernel code avoids the pollution by avoiding the named struct entirely, instead identifying the group's boundaries using either a pair of empty anonymous structs of a pair of zero-element arrays:

	struct foo {
		int one;
		struct { } start;
		int two;
		int three;
		struct { } finish;
		int four;
	};

	struct foo {
		int one;
		int start[0];
		int two;
		int three;
		int finish[0];
		int four;
	};

This allows code to avoid needing to use a sub-struct name for member references within the surrounding structure, but loses the benefits of being able to actually use such a struct, making it rather fragile. Using these requires open-coded calculation of sizes and offsets. The efforts made to avoid common mistakes include lots of comments, or adding various BUILD_BUG_ON()s. Such code is left with no way for the compiler to reason about the boundaries (e.g. the "start" object looks like it's 0 bytes in length and is not structurally associated with "finish"), making bounds checking depend on open-coded calculations:

	if (length > offsetof(struct foo, finish) -
		     offsetof(struct foo, start))
		return -EINVAL;
	memcpy(&dst.start, &src.start, length);

However, the vast majority of places in the kernel that operate on groups of members do so without any identification of the grouping, relying either on comments or implicit knowledge of the struct contents, which is even harder for the compiler to reason about, and results in even more fragile manual sizing, usually depending on member locations outside of the region (e.g. to copy "two" and "three", use the start of "four" to find the size):

	BUILD_BUG_ON((offsetof(struct foo, four) <
		      offsetof(struct foo, two)) ||
		     (offsetof(struct foo, four) <
		      offsetof(struct foo, three));
	if (length > offsetof(struct foo, four) -
		     offsetof(struct foo, two))
		return -EINVAL;
	memcpy(&dst.two, &src.two, length);

And both of the prior two idioms additionally appear to write beyond the end of the referenced struct member, forcing the compiler to ignore any attempt to perform bounds checking.

In order to have a regular programmatic way to describe a struct region that can be used for references and sizing, can be examined for bounds checking, avoids forcing the use of intermediate identifiers, and avoids polluting the global namespace, introduce the struct_group() macro. This macro wraps the member declarations to create an anonymous union of an anonymous struct (no intermediate name) and a named struct (for references and sizing):

	struct foo {
		int one;
		struct_group(thing,
			int two,
			int three,
		);
		int four;
	};

	if (length > sizeof(src.thing))
		return -EINVAL;
	memcpy(&dst.thing, &src.thing, length);
	do_something(dst.three);

There are some rare cases where the resulting struct_group() needs attributes added, so struct_group_attr() is also introduced to allow for specifying struct attributes (e.g. __align(x) or __packed).

Co-developed-by Keith Packard <[email protected]>

Signed-off-by Keith Packard <[email protected]>

Signed-off-by Kees Cook <[email protected]>

---
 include/linux/stddef.h | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

include/linux/stddef.h +34 -0

index 998a4ba28eba..cf7f866944f9 100644
@@ -36,4 +36,38 @@ enum {
 #define offsetofend(TYPE, MEMBER) \
 	(offsetof(TYPE, MEMBER)	+ sizeof_field(TYPE, MEMBER))
 
+/**
+ * struct_group_attr(NAME, ATTRS, MEMBERS)
+ *
+ * Used to create an anonymous union of two structs with identical
+ * layout and size: one anonymous and one named. The former can be
+ * used normally without sub-struct naming, and the latter can be
+ * used to reason about the start, end, and size of the group of
+ * struct members. Includes structure attributes argument.
+ *
+ * @NAME: The name of the mirrored sub-struct
+ * @ATTRS: Any struct attributes (normally empty)
+ * @MEMBERS: The member declarations for the mirrored structs
+ */
+#define struct_group_attr(NAME, ATTRS, MEMBERS) \
+	union { \
+		struct { MEMBERS } ATTRS; \
+		struct { MEMBERS } ATTRS NAME; \
+	}
+
+/**
+ * struct_group(NAME, MEMBERS)
+ *
+ * Used to create an anonymous union of two structs with identical
+ * layout and size: one anonymous and one named. The former can be
+ * used normally without sub-struct naming, and the latter can be
+ * used to reason about the start, end, and size of the group of
+ * struct members.
+ *
+ * @NAME: The name of the mirrored sub-struct
+ * @MEMBERS: The member declarations for the mirrored structs
+ */
+#define struct_group(NAME, MEMBERS)	\
+	struct_group_attr(NAME, /* no attrs */, MEMBERS)
+
 #endif

Signature · 1 line

2.30.2

In order to have a regular programmatic way to describe a struct region that can be used for references and sizing, can be examined for bounds checking, avoids forcing the use of intermediate identifiers, and avoids polluting the global namespace, introduce the struct_group() macro. This macro wraps the member declarations to create an anonymous union of an anonymous struct (no intermediate name) and a named struct (for references and sizing):

	struct foo {
		int one;
		struct_group(thing,
			int two,
			int three,
		);
		int four;
	};

Co-developed-by Keith Packard <[email protected]>

Signed-off-by Keith Packard <[email protected]>

Signed-off-by Kees Cook <[email protected]>

---
 include/linux/stddef.h | 34 ++++++++++++++++++++++++++++++++++

Quoted 8 lines · Kees Cook

In order to have a regular programmatic way to describe a struct region that can be used for references and sizing, can be examined for bounds checking, avoids forcing the use of intermediate identifiers, and avoids polluting the global namespace, introduce the struct_group() macro. This macro wraps the member declarations to create an anonymous union of an anonymous struct (no intermediate name) and a named struct (for references and sizing):

Quoted 8 lines

	struct foo {
		int one;
		struct_group(thing,
			int two,
			int three,
		);
		int four;
	};

That example won't compile, the commas after two and three should be semicolons.

And your implementation relies on MEMBERS not containing any comma tokens, but as

int a, b, c, d;

is a valid way to declare multiple members, consider making MEMBERS variadic

#define struct_group(NAME, MEMBERS...)

to have it slurp up every subsequent argument and make that work.

Quoted 7 lines

Co-developed-by Keith Packard <[email protected]>

Signed-off-by Keith Packard <[email protected]>

Signed-off-by Kees Cook <[email protected]>

---
 include/linux/stddef.h | 34 ++++++++++++++++++++++++++++++++++

Bikeshedding a bit, but do we need to add 34 lines that need to be preprocessed to virtually each and every translation unit [as opposed to adding a struct_group.h header]? Oh well, you need it for struct skbuff.h, so it would be pulled in by a lot regardless :(

Quoted 19 lines · Rasmus Villemoes

Quoted 8 lines · Kees Cook

In order to have a regular programmatic way to describe a struct region that can be used for references and sizing, can be examined for bounds checking, avoids forcing the use of intermediate identifiers, and avoids polluting the global namespace, introduce the struct_group() macro. This macro wraps the member declarations to create an anonymous union of an anonymous struct (no intermediate name) and a named struct (for references and sizing):

Quoted 8 lines

        struct foo {
                int one;
                struct_group(thing,
                        int two,
                        int three,
                );
                int four;
        };

Quoted 2 lines

That example won't compile, the commas after two and three should be semicolons.

Oops, yes, thanks. This is why I shouldn't write code that doesn't first go through a compiler. ;)

Quoted 5 lines

And your implementation relies on MEMBERS not containing any comma tokens, but as

  int a, b, c, d;

Quoted 6 lines

is a valid way to declare multiple members, consider making MEMBERS variadic

#define struct_group(NAME, MEMBERS...)

to have it slurp up every subsequent argument and make that work.

Ah! Perfect, thank you. I totally forgot I could do it that way.

Quoted 36 lines · Kees Cook

Quoted 19 lines · Rasmus Villemoes

Quoted 8 lines · Kees Cook

In order to have a regular programmatic way to describe a struct region that can be used for references and sizing, can be examined for bounds checking, avoids forcing the use of intermediate identifiers, and avoids polluting the global namespace, introduce the struct_group() macro. This macro wraps the member declarations to create an anonymous union of an anonymous struct (no intermediate name) and a named struct (for references and sizing):

Quoted 8 lines

        struct foo {
                int one;
                struct_group(thing,
                        int two,
                        int three,
                );
                int four;
        };

Quoted 2 lines

That example won't compile, the commas after two and three should be semicolons.

Oops, yes, thanks. This is why I shouldn't write code that doesn't first go through a compiler. ;)

Quoted 10 lines

And your implementation relies on MEMBERS not containing any comma tokens, but as

  int a, b, c, d;

is a valid way to declare multiple members, consider making MEMBERS variadic

#define struct_group(NAME, MEMBERS...)

Quoted 3 lines

Quoted 1 line

to have it slurp up every subsequent argument and make that work.

Ah! Perfect, thank you. I totally forgot I could do it that way.

This is great Kees. It just so happens it would clean-up what we are already doing in drivers/cxl/cxl.h for anonymous + named register block pointers. However in the cxl case it also needs the named structure to be typed. Any appetite for a typed version of this?

Here is a rough idea of the cleanup it would induce in drivers/cxl/:

drivers/cxl/cxl.h +6 -39

index 53927f9fa77e..a2308c995654 100644
@@ -75,52 +75,19 @@ static inline int cxl_hdm_decoder_count(u32 cap_hdr)
 #define CXLDEV_MBOX_BG_CMD_STATUS_OFFSET 0x18
 #define CXLDEV_MBOX_PAYLOAD_OFFSET 0x20
 
-#define CXL_COMPONENT_REGS() \
-       void __iomem *hdm_decoder
-
-#define CXL_DEVICE_REGS() \
-       void __iomem *status; \
-       void __iomem *mbox; \
-       void __iomem *memdev
-
-/* See note for 'struct cxl_regs' for the rationale of this organization */
 /*
- * CXL_COMPONENT_REGS - Common set of CXL Component register block base pointers
  * @hdm_decoder: CXL 2.0 8.2.5.12 CXL HDM Decoder Capability Structure
- */
-struct cxl_component_regs {
-       CXL_COMPONENT_REGS();
-};
-
-/* See note for 'struct cxl_regs' for the rationale of this organization */
-/*
- * CXL_DEVICE_REGS - Common set of CXL Device register block base pointers
  * @status: CXL 2.0 8.2.8.3 Device Status Registers
  * @mbox: CXL 2.0 8.2.8.4 Mailbox Registers
  * @memdev: CXL 2.0 8.2.8.5 Memory Device Registers
  */
-struct cxl_device_regs {
-       CXL_DEVICE_REGS();
-};
-
-/*
- * Note, the anonymous union organization allows for per
- * register-block-type helper routines, without requiring block-type
- * agnostic code to include the prefix.
- */
 struct cxl_regs {
-       union {
-               struct {
-                       CXL_COMPONENT_REGS();
-               };
-               struct cxl_component_regs component;
-       };
-       union {
-               struct {
-                       CXL_DEVICE_REGS();
-               };
-               struct cxl_device_regs device_regs;
-       };
+       struct_group_typed(cxl_component_regs, component,
+               void __iomem *hdm_decoder;
+       );
+       struct_group_typed(cxl_device_regs, device_regs,
+               void __iomem *status, *mbox, *memdev;
+       );
 };
 
 struct cxl_reg_map {

include/linux/stddef.h +7 -1

index cf7f866944f9..84b7de24ffb5 100644
@@ -49,12 +49,18 @@ enum {
  * @ATTRS: Any struct attributes (normally empty)
  * @MEMBERS: The member declarations for the mirrored structs
  */
-#define struct_group_attr(NAME, ATTRS, MEMBERS) \
+#define struct_group_attr(NAME, ATTRS, MEMBERS...) \
        union { \
                struct { MEMBERS } ATTRS; \
                struct { MEMBERS } ATTRS NAME; \
        }
 
+#define struct_group_attr_typed(TYPE, NAME, ATTRS, MEMBERS...) \
+       union { \
+               struct { MEMBERS } ATTRS; \
+               struct TYPE { MEMBERS } ATTRS NAME; \
+       }
+
 /**
  * struct_group(NAME, MEMBERS)
  *

@@ -67,7 +73,10 @@ enum {
  * @NAME: The name of the mirrored sub-struct

Quoted 7 lines · Dan J

On Wed, 2021-07-28 at 14:59 -0700, Kees Cook wrote:

/**
* struct_group(NAME, MEMBERS)
*

include/linux/stddef.h +0 -0

@@ -67,7 +73,10 @@ enum {
  * @NAME: The name of the mirrored sub-struct

include/linux/stddef.h enum { · line 82

Quoted 10 lines

include/linux/stddef.h +4 -1

  * @MEMBERS: The member declarations for the mirrored structs
  */
-#define struct_group(NAME, MEMBERS)    \
+#define struct_group(NAME, MEMBERS...) \
        struct_group_attr(NAME, /* no attrs */, MEMBERS)
 
+#define struct_group_typed(TYPE, NAME, MEMBERS...) \
+       struct_group_attr_typed(TYPE, NAME, /* no attrs */, MEMBERS)
+
 #endif

Awesome! My instinct is to expose the resulting API as:

__struct_group(type, name, attrs, members...)

struct_group(name, members...)
struct_group_attr(name, attrs, members...)
struct_group_typed(type, name, members...)

Quoted 21 lines · On Sat, Jul 31, 2021, 04:59 Kees Cook <[email protected]> wrote:

Quoted 7 lines · Dan J

On Wed, 2021-07-28 at 14:59 -0700, Kees Cook wrote:

/**
* struct_group(NAME, MEMBERS)
*

include/linux/stddef.h +0 -0

@@ -67,7 +73,10 @@ enum {
  * @NAME: The name of the mirrored sub-struct

include/linux/stddef.h enum { · line 82

Quoted 10 lines

include/linux/stddef.h +4 -1

  * @MEMBERS: The member declarations for the mirrored structs
  */
-#define struct_group(NAME, MEMBERS)    \
+#define struct_group(NAME, MEMBERS...) \
        struct_group_attr(NAME, /* no attrs */, MEMBERS)
 
+#define struct_group_typed(TYPE, NAME, MEMBERS...) \
+       struct_group_attr_typed(TYPE, NAME, /* no attrs */, MEMBERS)
+
 #endif

Awesome! My instinct is to expose the resulting API as:

Quoted 5 lines

__struct_group(type, name, attrs, members...)

struct_group(name, members...)
struct_group_attr(name, attrs, members...)
struct_group_typed(type, name, members...)

Bikeshed: can we use proper nomenclature please. s/type/tag/, s/typed/tagged.

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.

Replace the existing empty member position markers "headers_start" and "headers_end" with a struct_group(). This will allow memcpy() and sizeof() to more easily reason about sizes, and improve readability.

"pahole" shows no size nor member offset changes to struct sk_buff. "objdump -d" shows no no meaningful object code changes (i.e. only source line number induced differences and optimizations.)

Signed-off-by Kees Cook <[email protected]>

---
 drivers/net/wireguard/queueing.h |  4 +---
 include/linux/skbuff.h           |  9 ++++-----
 net/core/skbuff.c                | 14 +++++---------
 3 files changed, 10 insertions(+), 17 deletions(-)

drivers/net/wireguard/queueing.h +1 -3

index 4ef2944a68bc..52da5e963003 100644
@@ -79,9 +79,7 @@ static inline void wg_reset_packet(struct sk_buff *skb, bool encapsulating)
 	u8 sw_hash = skb->sw_hash;
 	u32 hash = skb->hash;
 	skb_scrub_packet(skb, true);
-	memset(&skb->headers_start, 0,
-	       offsetof(struct sk_buff, headers_end) -
-		       offsetof(struct sk_buff, headers_start));
+	memset(&skb->headers, 0, sizeof(skb->headers));
 	if (encapsulating) {
 		skb->l4_hash = l4_hash;
 		skb->sw_hash = sw_hash;

include/linux/skbuff.h +4 -5

index f19190820e63..b4032e9b130e 100644
@@ -800,11 +800,10 @@ struct sk_buff {
 	__u8			active_extensions;
 #endif
 
-	/* fields enclosed in headers_start/headers_end are copied
+	/* Fields enclosed in headers group are copied
 	 * using a single memcpy() in __copy_skb_header()
 	 */
-	/* private: */
-	__u32			headers_start[0];
+	struct_group(headers,
 	/* public: */
 
 /* if you move pkt_type around you also must adapt those constants */

@@ -920,8 +919,8 @@ struct sk_buff {
 	u64			kcov_handle;
 #endif
 
-	/* private: */
-	__u32			headers_end[0];
+	); /* end headers group */
+
 	/* public: */
 
 	/* These elements must be at the end, see alloc_skb() for details.  */

net/core/skbuff.c +5 -9

index fc7942c0dddc..5f29c65507e0 100644
@@ -987,12 +987,10 @@ void napi_consume_skb(struct sk_buff *skb, int budget)
 }
 EXPORT_SYMBOL(napi_consume_skb);
 
-/* Make sure a field is enclosed inside headers_start/headers_end section */
+/* Make sure a field is contained by headers group */
 #define CHECK_SKB_FIELD(field) \
-	BUILD_BUG_ON(offsetof(struct sk_buff, field) <		\
-		     offsetof(struct sk_buff, headers_start));	\
-	BUILD_BUG_ON(offsetof(struct sk_buff, field) >		\
-		     offsetof(struct sk_buff, headers_end));	\
+	BUILD_BUG_ON(offsetof(struct sk_buff, field) !=		\
+		     offsetof(struct sk_buff, headers.field));	\
 
 static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old)
 {

@@ -1004,14 +1002,12 @@ static void __copy_skb_header(struct sk_buff *new, const struct sk_buff *old)
 	__skb_ext_copy(new, old);
 	__nf_copy(new, old, false);
 
-	/* Note : this field could be in headers_start/headers_end section
+	/* Note : this field could be in the headers group.
 	 * It is not yet because we do not want to have a 16 bit hole
 	 */
 	new->queue_mapping = old->queue_mapping;
 
-	memcpy(&new->headers_start, &old->headers_start,
-	       offsetof(struct sk_buff, headers_end) -
-	       offsetof(struct sk_buff, headers_start));
+	memcpy(&new->headers, &old->headers, sizeof(new->headers));
 	CHECK_SKB_FIELD(protocol);
 	CHECK_SKB_FIELD(csum);
 	CHECK_SKB_FIELD(hash);

Signature · 1 line

2.30.2

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.

Use struct_group() around members queue_id, min_bw, max_bw, tsa, pri_lvl, and bw_weight so they can be referenced together. This will allow memcpy() and sizeof() to more easily reason about sizes, improve readability, and avoid future warnings about writing beyond the end of queue_id.

"pahole" shows no size nor member offset changes to struct bnxt_cos2bw_cfg. "objdump -d" shows no meaningful object code changes (i.e. only source line number induced differences and optimizations).

Signed-off-by Kees Cook <[email protected]>

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.

Use struct_group() around members queue_id, min_bw, max_bw, tsa, pri_lvl, and bw_weight so they can be referenced together. This will allow memcpy() and sizeof() to more easily reason about sizes, improve readability, and avoid future warnings about writing beyond the end of queue_id.

"pahole" shows no size nor member offset changes to struct bnxt_cos2bw_cfg. "objdump -d" shows no meaningful object code changes (i.e. only source line number induced differences and optimizations).

Signed-off-by Kees Cook <[email protected]>

---
 drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c |  4 ++--
 drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.h | 14 ++++++++------
 2 files changed, 10 insertions(+), 8 deletions(-)

drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.c +2 -2

index 8a68df4d9e59..95c636f89329 100644
@@ -148,10 +148,10 @@ static int bnxt_hwrm_queue_cos2bw_qcfg(struct bnxt *bp, struct ieee_ets *ets)
 	}
 
 	data = &resp->queue_id0 + offsetof(struct bnxt_cos2bw_cfg, queue_id);
-	for (i = 0; i < bp->max_tc; i++, data += sizeof(cos2bw) - 4) {
+	for (i = 0; i < bp->max_tc; i++, data += sizeof(cos2bw.cfg)) {
 		int tc;
 
-		memcpy(&cos2bw.queue_id, data, sizeof(cos2bw) - 4);
+		memcpy(&cos2bw.cfg, data, sizeof(cos2bw.cfg));
 		if (i == 0)
 			cos2bw.queue_id = resp->queue_id0;
 

drivers/net/ethernet/broadcom/bnxt/bnxt_dcb.h +8 -6

index 6eed231de565..716742522161 100644
@@ -23,13 +23,15 @@ struct bnxt_dcb {
 
 struct bnxt_cos2bw_cfg {
 	u8			pad[3];
-	u8			queue_id;
-	__le32			min_bw;
-	__le32			max_bw;
+	struct_group_attr(cfg, __packed,
+		u8		queue_id;
+		__le32		min_bw;
+		__le32		max_bw;
 #define BW_VALUE_UNIT_PERCENT1_100		(0x1UL << 29)
-	u8			tsa;
-	u8			pri_lvl;
-	u8			bw_weight;
+		u8		tsa;
+		u8		pri_lvl;
+		u8		bw_weight;
+	);
 	u8			unused;
 };
 

Signature · 1 line

2.30.2

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.

Use struct_group() around members addr1, addr2, and addr3 in struct rtllib_hdr_4addr, and members qui, qui_type, qui_subtype, version, and ac_info in struct rtllib_qos_information_element, so they can be referenced together. This will allow memcpy() and sizeof() to more easily reason about sizes, improve readability, and avoid future warnings about writing beyond the end of addr1 and qui.

"pahole" shows no size nor member offset changes to struct rtllib_hdr_4addr nor struct rtllib_qos_information_element. "objdump -d" shows no meaningful object code changes (i.e. only source line number induced differences and optimizations).

Signed-off-by Kees Cook<[email protected]>

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.

Use struct_group() around members addr1, addr2, and addr3 in struct rtllib_hdr_4addr, and members qui, qui_type, qui_subtype, version, and ac_info in struct rtllib_qos_information_element, so they can be referenced together. This will allow memcpy() and sizeof() to more easily reason about sizes, improve readability, and avoid future warnings about writing beyond the end of addr1 and qui.

"pahole" shows no size nor member offset changes to struct rtllib_hdr_4addr nor struct rtllib_qos_information_element. "objdump -d" shows no meaningful object code changes (i.e. only source line number induced differences and optimizations).

Signed-off-by Kees Cook <[email protected]>

---
 drivers/staging/rtl8192e/rtllib.h            | 20 ++++++++++++--------
 drivers/staging/rtl8192e/rtllib_crypt_ccmp.c |  3 ++-
 drivers/staging/rtl8192e/rtllib_rx.c         |  8 ++++----
 3 files changed, 18 insertions(+), 13 deletions(-)

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.

Use struct_group() around members addr1, addr2, and addr3 in struct rtl_80211_hdr_4addr, and members qui, qui_type, qui_subtype, version, and ac_info in struct ieee80211_qos_information_element, so they can be referenced together. This will allow memcpy() and sizeof() to more easily reason about sizes, improve readability, and avoid future warnings about writing beyond the end of addr1 and qui. Additionally replace zero sized arrays with flexible arrays in struct ieee_param.

"pahole" shows no size nor member offset changes to struct rtl_80211_hdr_4addr nor struct ieee80211_qos_information_element. "objdump -d" shows no meaningful object code changes (i.e. only source line number induced differences and optimizations).

Signed-off-by Kees Cook <[email protected]>

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.

Adjust memcpy() destination to be the named structure itself, rather than the first member, allowing memcpy() to correctly reason about the size.

"objdump -d" shows no object code changes.

Signed-off-by Kees Cook <[email protected]>

---
 drivers/staging/rtl8723bs/core/rtw_mlme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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.

Use struct_group() around members addr1, addr2, and addr3 in struct ieee80211_hdr so they can be referenced together. This will allow memcpy() and sizeof() to more easily reason about sizes, improve readability, and avoid future warnings about writing beyond the end of addr1.

"pahole" shows no size nor member offset changes to struct ieee80211_hdr. "objdump -d" shows no meaningful object code changes (i.e. only source line number induced differences and optimizations).

Signed-off-by Kees Cook <[email protected]>

---
 drivers/staging/rtl8723bs/core/rtw_security.c | 5 +++--
 drivers/staging/rtl8723bs/core/rtw_xmit.c     | 5 +++--
 include/linux/ieee80211.h                     | 8 +++++---
 net/wireless/lib80211_crypt_ccmp.c            | 3 ++-
 4 files changed, 13 insertions(+), 8 deletions(-)

+++ b/include/linux/ieee80211.h

include/linux/ieee80211.h +0 -1

@@ -297,9 +297,11 @@ static inline u16 ieee80211_sn_sub(u16 sn1, u16 sn2)
  struct ieee80211_hdr {
  	__le16 frame_control;
  	__le16 duration_id;
-	u8 addr1[ETH_ALEN];

Quoted 7 lines · Kees Cook

+++ b/include/linux/ieee80211.h

include/linux/ieee80211.h +0 -1

@@ -297,9 +297,11 @@ static inline u16 ieee80211_sn_sub(u16 sn1, u16 sn2)
  struct ieee80211_hdr {
  	__le16 frame_control;
  	__le16 duration_id;
-	u8 addr1[ETH_ALEN];

include/linux/ieee80211.h static inline u16 ieee80211_sn_sub(u16 sn1, u16 sn2) · line 307

Quoted 10 lines

include/linux/ieee80211.h +5 -2

-	u8 addr2[ETH_ALEN];
-	u8 addr3[ETH_ALEN];
+	struct_group(addrs,
+		u8 addr1[ETH_ALEN];
+		u8 addr2[ETH_ALEN];
+		u8 addr3[ETH_ALEN];
+	);
  	__le16 seq_ctrl;
  	u8 addr4[ETH_ALEN];
  } __packed __aligned(2);

This file isn't really just lib80211, it's also used by everyone else for 802.11, but I guess that's OK - after all, this doesn't really result in any changes here.

net/wireless/lib80211_crypt_ccmp.c static int ccmp_init_iv_and_aad(const struct ieee80211_hdr *hdr, · line 140

Quoted 8 lines

+++ b/net/wireless/lib80211_crypt_ccmp.c

net/wireless/lib80211_crypt_ccmp.c +2 -1

@@ -136,7 +136,8 @@ static int ccmp_init_iv_and_aad(const struct ieee80211_hdr *hdr,
  	pos = (u8 *) hdr;
  	aad[0] = pos[0] & 0x8f;
  	aad[1] = pos[1] & 0xc7;
-	memcpy(aad + 2, hdr->addr1, 3 * ETH_ALEN);
+	BUILD_BUG_ON(sizeof(hdr->addrs) != 3 * ETH_ALEN);
+	memcpy(aad + 2, &hdr->addrs, ETH_ALEN);

However, how is it you don't need the same change in net/mac80211/wpa.c?

We have three similar instances:

        /* AAD (extra authenticate-only data) / masked 802.11 header
         * FC | A1 | A2 | A3 | SC | [A4] | [QC] */
        put_unaligned_be16(len_a, &aad[0]);
        put_unaligned(mask_fc, (__le16 *)&aad[2]);
        memcpy(&aad[4], &hdr->addr1, 3 * ETH_ALEN);

and

        memcpy(&aad[4], &hdr->addr1, 3 * ETH_ALEN);

and

        memcpy(aad + 2, &hdr->addr1, 3 * ETH_ALEN);

so those should also be changed, it seems?

In which case I'd probably prefer to do this separately from the staging drivers ...

Ah! Yes, thanks for pointing this out. During earlier development I split the "cross-field write" changes from the "cross-field read" changes, and it looks like I missed moving lib80211_crypt_ccmp.c into that portion of the series (which I haven't posted nor finished -- it's lower priority than fixing the cross-field writes).

In preparation for FORTIFY_SOURCE performing compile-time and run-time field array bounds checking for memcpy(), memmove(), and memset(), avoid intentionally writing across neighboring fields.

Use struct_group() in struct libipw_qos_information_element around members qui, qui_type, qui_subtype, version, and ac_info, so they can be referenced together. This will allow memcpy() and sizeof() to more easily reason about sizes, improve readability, and avoid future warnings about writing beyond the end of qui.

"pahole" shows no size nor member offset changes to struct libipw_qos_information_element.

Additionally corrects the size in libipw_read_qos_param_element() as it was testing the wrong structure size (it should have been struct libipw_qos_information_element, not struct libipw_qos_parameter_info).

Signed-off-by Kees Cook <[email protected]>

---
 drivers/net/wireless/intel/ipw2x00/libipw.h    | 12 +++++++-----
 drivers/net/wireless/intel/ipw2x00/libipw_rx.c |  8 ++++----
 2 files changed, 11 insertions(+), 9 deletions(-)