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:
quoted
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 <redacted>
---
drivers/media/platform/omap3isp/ispstat.c | 5 +--
include/uapi/linux/omap3isp.h | 44 +++++++++++++++++------
2 files changed, 36 insertions(+), 13 deletions(-)
From: "Gustavo A. R. Silva" <gustavoars@kernel.org> Date: 2021-07-28 02:05:20
On Tue, Jul 27, 2021 at 01:57:54PM -0700, Kees Cook wrote:
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 <redacted>
Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Thanks
--
Gustavo
From: "Gustavo A. R. Silva" <gustavoars@kernel.org> Date: 2021-07-28 02:29:55
On Tue, Jul 27, 2021 at 01:57:55PM -0700, Kees Cook wrote:
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 <redacted>
Signed-off-by: Keith Packard <redacted>
Signed-off-by: Kees Cook <redacted>
Acked-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Love it! :)
Thanks
--
Gustavo
From: "Gustavo A. R. Silva" <gustavoars@kernel.org> Date: 2021-07-28 03:47:40
On Tue, Jul 27, 2021 at 01:57:56PM -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.
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 <redacted>
Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Thanks
--
Gustavo
@@ -800,11 +800,10 @@ struct sk_buff {__u8active_extensions;#endif-/* fields enclosed in headers_start/headers_end are copied+/* Fields enclosed in headers group are copied*usingasinglememcpy()in__copy_skb_header()*/-/* private: */-__u32headers_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 {u64kcov_handle;#endif-/* private: */-__u32headers_end[0];+);/* end headers group */+/* public: *//* These elements must be at the end, see alloc_skb() for details. */
@@ -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(structsk_buff,field)<\-offsetof(structsk_buff,headers_start));\-BUILD_BUG_ON(offsetof(structsk_buff,field)>\-offsetof(structsk_buff,headers_end));\+BUILD_BUG_ON(offsetof(structsk_buff,field)!=\+offsetof(structsk_buff,headers.field));\staticvoid__copy_skb_header(structsk_buff*new,conststructsk_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.*Itisnotyetbecausewedonotwanttohavea16bithole*/new->queue_mapping=old->queue_mapping;-memcpy(&new->headers_start,&old->headers_start,-offsetof(structsk_buff,headers_end)--offsetof(structsk_buff,headers_start));+memcpy(&new->headers,&old->headers,sizeof(new->headers));CHECK_SKB_FIELD(protocol);CHECK_SKB_FIELD(csum);CHECK_SKB_FIELD(hash);
From: "Gustavo A. R. Silva" <gustavoars@kernel.org> Date: 2021-07-28 04:42:50
On Tue, Jul 27, 2021 at 01:57:57PM -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.
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 <redacted>
Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org>
Thanks
--
Gustavo
On Tue, Jul 27, 2021 at 01:57:58PM -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.
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 <redacted>
---
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(-)
On Tue, Jul 27, 2021 at 01:58:15PM -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.
Use struct_group() in struct hfa384x_tx_frame around members
frame_control, duration_id, address[1-4], and sequence_control, 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 frame_control.
"pahole" shows no size nor member offset changes to struct
hfa384x_tx_frame. "objdump -d" shows no meaningful object code changes
(i.e. only source line number induced differences.)
Signed-off-by: Kees Cook <redacted>
---
drivers/staging/wlan-ng/hfa384x.h | 16 +++++++++-------
drivers/staging/wlan-ng/hfa384x_usb.c | 4 +++-
2 files changed, 12 insertions(+), 8 deletions(-)
On Tue, Jul 27, 2021 at 01:57:59PM -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.
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 <redacted>
On Tue, Jul 27, 2021 at 01:58:00PM -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.
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 <redacted>
---
drivers/staging/rtl8723bs/core/rtw_mlme.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
On Tue, Jul 27, 2021 at 01:58:53PM -0700, Kees Cook wrote:
quoted hunk
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.
Add a flexible array member to mark the end of struct nlmsghdr, and
split the memcpy() to avoid false positive memcpy() warning:
memcpy: detected field-spanning write (size 32) of single field (size 16)
Signed-off-by: Kees Cook <redacted>
---
include/uapi/linux/netlink.h | 1 +
net/netlink/af_netlink.c | 4 +++-
2 files changed, 4 insertions(+), 1 deletion(-)
@@ -47,6 +47,7 @@ struct nlmsghdr {__u16nlmsg_flags;/* Additional flags */__u32nlmsg_seq;/* Sequence number */__u32nlmsg_pid;/* Sending process port ID */+__u8contents[];
Is this ok to change a public, userspace visable, structure?
Nothing breaks?
thanks,
greg k-h
On Tue, Jul 27, 2021 at 01:58:40PM -0700, Kees Cook wrote:
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Add struct_group() to mark region of struct cm4000_dev that should be
initialized to zero.
Signed-off-by: Kees Cook <redacted>
---
drivers/char/pcmcia/cm4000_cs.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
On Tue, Jul 27, 2021 at 01:58:01PM -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.
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 <redacted>
---
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(-)
For the staging portion:
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Tue, Jul 27, 2021 at 01:58:10PM -0700, Kees Cook wrote:
quoted hunk
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() in struct flowi4, struct ipv4hdr, and struct ipv6hdr
around members saddr and daddr, 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 saddr.
"pahole" shows no size nor member offset changes to struct flowi4.
"objdump -d" shows no meaningful object code changes (i.e. only source
line number induced differences.)
Note that since this is a UAPI header, struct_group() has been open
coded.
Signed-off-by: Kees Cook <redacted>
---
include/net/flow.h | 6 ++++--
include/uapi/linux/if_ether.h | 12 ++++++++++--
include/uapi/linux/ip.h | 12 ++++++++++--
include/uapi/linux/ipv6.h | 12 ++++++++++--
net/core/flow_dissector.c | 10 ++++++----
net/ipv4/ip_output.c | 6 ++----
6 files changed, 42 insertions(+), 16 deletions(-)
@@ -81,8 +81,10 @@ struct flowi4 {#define flowi4_multipath_hash __fl_common.flowic_multipath_hash/* (saddr,daddr) must be grouped, same order as in IP header */-__be32saddr;-__be32daddr;+struct_group(addrs,+__be32saddr;+__be32daddr;+);unionflowi_uliuli;#define fl4_sport uli.ports.sport
@@ -163,8 +163,16 @@#if __UAPI_DEF_ETHHDRstructethhdr{-unsignedcharh_dest[ETH_ALEN];/* destination eth addr */-unsignedcharh_source[ETH_ALEN];/* source ether addr */+union{+struct{+unsignedcharh_dest[ETH_ALEN];/* destination eth addr */+unsignedcharh_source[ETH_ALEN];/* source ether addr */+};+struct{+unsignedcharh_dest[ETH_ALEN];/* destination eth addr */+unsignedcharh_source[ETH_ALEN];/* source ether addr */+}addrs;
A union of the same fields in the same structure in the same way?
Ah, because struct_group() can not be used here? Still feels odd to see
in a userspace-visible header.
quoted hunk
+ };
__be16 h_proto; /* packet type ID field */
} __attribute__((packed));
#endif
On Tue, Jul 27, 2021 at 01:58:16PM -0700, Kees Cook wrote:
quoted hunk
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() in struct drm32_mga_init around members chipset, sgram,
maccess, fb_cpp, front_offset, front_pitch, back_offset, back_pitch,
depth_cpp, depth_offset, depth_pitch, texture_offset, and texture_size,
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 chipset.
"pahole" shows no size nor member offset changes to struct drm32_mga_init.
"objdump -d" shows no meaningful object code changes (i.e. only source
line number induced differences and optimizations).
Note that since this includes a UAPI header, struct_group() has been
explicitly redefined local to the header.
Signed-off-by: Kees Cook <redacted>
---
drivers/gpu/drm/mga/mga_ioc32.c | 30 ++++++++++++++------------
include/uapi/drm/mga_drm.h | 37 ++++++++++++++++++++++++---------
2 files changed, 44 insertions(+), 23 deletions(-)
On Wed, Jul 28, 2021 at 01:14:33AM -0500, Gustavo A. R. Silva wrote:
On 7/28/21 00:55, Greg Kroah-Hartman wrote:
quoted
On Tue, Jul 27, 2021 at 01:58:10PM -0700, Kees Cook wrote:
quoted
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() in struct flowi4, struct ipv4hdr, and struct ipv6hdr
around members saddr and daddr, 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 saddr.
"pahole" shows no size nor member offset changes to struct flowi4.
"objdump -d" shows no meaningful object code changes (i.e. only source
line number induced differences.)
Note that since this is a UAPI header, struct_group() has been open
coded.
Signed-off-by: Kees Cook <redacted>
---
include/net/flow.h | 6 ++++--
include/uapi/linux/if_ether.h | 12 ++++++++++--
include/uapi/linux/ip.h | 12 ++++++++++--
include/uapi/linux/ipv6.h | 12 ++++++++++--
net/core/flow_dissector.c | 10 ++++++----
net/ipv4/ip_output.c | 6 ++----
6 files changed, 42 insertions(+), 16 deletions(-)
@@ -81,8 +81,10 @@ struct flowi4 {#define flowi4_multipath_hash __fl_common.flowic_multipath_hash/* (saddr,daddr) must be grouped, same order as in IP header */-__be32saddr;-__be32daddr;+struct_group(addrs,+__be32saddr;+__be32daddr;+);unionflowi_uliuli;#define fl4_sport uli.ports.sport
@@ -163,8 +163,16 @@#if __UAPI_DEF_ETHHDRstructethhdr{-unsignedcharh_dest[ETH_ALEN];/* destination eth addr */-unsignedcharh_source[ETH_ALEN];/* source ether addr */+union{+struct{+unsignedcharh_dest[ETH_ALEN];/* destination eth addr */+unsignedcharh_source[ETH_ALEN];/* source ether addr */+};+struct{+unsignedcharh_dest[ETH_ALEN];/* destination eth addr */+unsignedcharh_source[ETH_ALEN];/* source ether addr */+}addrs;
A union of the same fields in the same structure in the same way?
Ah, because struct_group() can not be used here? Still feels odd to see
in a userspace-visible header.
quoted
+ };
__be16 h_proto; /* packet type ID field */
} __attribute__((packed));
#endif
I think addrs should be second. In general, I think all newly added
non-anonymous structures should be second.
Why not use a local version of the macro like was done in the DRM header
file, to make it always work the same and more obvious what is
happening? If I were a userspace developer and saw the above, I would
think that the kernel developers have lost it :)
thanks,
greg k-h
On Wed, Jul 28, 2021 at 01:31:16AM -0500, Gustavo A. R. Silva wrote:
quoted
Why not use a local version of the macro like was done in the DRM header
file, to make it always work the same and more obvious what is
happening? If I were a userspace developer and saw the above, I would
think that the kernel developers have lost it :)
From: Gustavo A. R. Silva <hidden> Date: 2021-07-28 06:50:22
On 7/28/21 01:19, Greg Kroah-Hartman wrote:
On Wed, Jul 28, 2021 at 01:14:33AM -0500, Gustavo A. R. Silva wrote:
quoted
On 7/28/21 00:55, Greg Kroah-Hartman wrote:
quoted
On Tue, Jul 27, 2021 at 01:58:10PM -0700, Kees Cook wrote:
quoted
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() in struct flowi4, struct ipv4hdr, and struct ipv6hdr
around members saddr and daddr, 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 saddr.
"pahole" shows no size nor member offset changes to struct flowi4.
"objdump -d" shows no meaningful object code changes (i.e. only source
line number induced differences.)
Note that since this is a UAPI header, struct_group() has been open
coded.
Signed-off-by: Kees Cook <redacted>
---
include/net/flow.h | 6 ++++--
include/uapi/linux/if_ether.h | 12 ++++++++++--
include/uapi/linux/ip.h | 12 ++++++++++--
include/uapi/linux/ipv6.h | 12 ++++++++++--
net/core/flow_dissector.c | 10 ++++++----
net/ipv4/ip_output.c | 6 ++----
6 files changed, 42 insertions(+), 16 deletions(-)
@@ -81,8 +81,10 @@ struct flowi4 {#define flowi4_multipath_hash __fl_common.flowic_multipath_hash/* (saddr,daddr) must be grouped, same order as in IP header */-__be32saddr;-__be32daddr;+struct_group(addrs,+__be32saddr;+__be32daddr;+);unionflowi_uliuli;#define fl4_sport uli.ports.sport
@@ -163,8 +163,16 @@#if __UAPI_DEF_ETHHDRstructethhdr{-unsignedcharh_dest[ETH_ALEN];/* destination eth addr */-unsignedcharh_source[ETH_ALEN];/* source ether addr */+union{+struct{+unsignedcharh_dest[ETH_ALEN];/* destination eth addr */+unsignedcharh_source[ETH_ALEN];/* source ether addr */+};+struct{+unsignedcharh_dest[ETH_ALEN];/* destination eth addr */+unsignedcharh_source[ETH_ALEN];/* source ether addr */+}addrs;
A union of the same fields in the same structure in the same way?
Ah, because struct_group() can not be used here? Still feels odd to see
in a userspace-visible header.
quoted
+ };
__be16 h_proto; /* packet type ID field */
} __attribute__((packed));
#endif
I think addrs should be second. In general, I think all newly added
non-anonymous structures should be second.
Why not use a local version of the macro like was done in the DRM header
file, to make it always work the same and more obvious what is
happening? If I were a userspace developer and saw the above, I would
think that the kernel developers have lost it :)
From: Gustavo A. R. Silva <hidden> Date: 2021-07-28 06:58:26
On 7/28/21 01:31, Gustavo A. R. Silva wrote:
On 7/28/21 01:19, Greg Kroah-Hartman wrote:
quoted
On Wed, Jul 28, 2021 at 01:14:33AM -0500, Gustavo A. R. Silva wrote:
quoted
On 7/28/21 00:55, Greg Kroah-Hartman wrote:
quoted
On Tue, Jul 27, 2021 at 01:58:10PM -0700, Kees Cook wrote:
quoted
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() in struct flowi4, struct ipv4hdr, and struct ipv6hdr
around members saddr and daddr, 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 saddr.
"pahole" shows no size nor member offset changes to struct flowi4.
"objdump -d" shows no meaningful object code changes (i.e. only source
line number induced differences.)
Note that since this is a UAPI header, struct_group() has been open
coded.
Signed-off-by: Kees Cook <redacted>
---
include/net/flow.h | 6 ++++--
include/uapi/linux/if_ether.h | 12 ++++++++++--
include/uapi/linux/ip.h | 12 ++++++++++--
include/uapi/linux/ipv6.h | 12 ++++++++++--
net/core/flow_dissector.c | 10 ++++++----
net/ipv4/ip_output.c | 6 ++----
6 files changed, 42 insertions(+), 16 deletions(-)
@@ -81,8 +81,10 @@ struct flowi4 {#define flowi4_multipath_hash __fl_common.flowic_multipath_hash/* (saddr,daddr) must be grouped, same order as in IP header */-__be32saddr;-__be32daddr;+struct_group(addrs,+__be32saddr;+__be32daddr;+);unionflowi_uliuli;#define fl4_sport uli.ports.sport
@@ -163,8 +163,16 @@#if __UAPI_DEF_ETHHDRstructethhdr{-unsignedcharh_dest[ETH_ALEN];/* destination eth addr */-unsignedcharh_source[ETH_ALEN];/* source ether addr */+union{+struct{+unsignedcharh_dest[ETH_ALEN];/* destination eth addr */+unsignedcharh_source[ETH_ALEN];/* source ether addr */+};+struct{+unsignedcharh_dest[ETH_ALEN];/* destination eth addr */+unsignedcharh_source[ETH_ALEN];/* source ether addr */+}addrs;
A union of the same fields in the same structure in the same way?
Ah, because struct_group() can not be used here? Still feels odd to see
in a userspace-visible header.
quoted
+ };
__be16 h_proto; /* packet type ID field */
} __attribute__((packed));
#endif
From: Gustavo A. R. Silva <hidden> Date: 2021-07-28 06:58:36
On 7/28/21 00:55, Greg Kroah-Hartman wrote:
On Tue, Jul 27, 2021 at 01:58:10PM -0700, Kees Cook wrote:
quoted
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() in struct flowi4, struct ipv4hdr, and struct ipv6hdr
around members saddr and daddr, 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 saddr.
"pahole" shows no size nor member offset changes to struct flowi4.
"objdump -d" shows no meaningful object code changes (i.e. only source
line number induced differences.)
Note that since this is a UAPI header, struct_group() has been open
coded.
Signed-off-by: Kees Cook <redacted>
---
include/net/flow.h | 6 ++++--
include/uapi/linux/if_ether.h | 12 ++++++++++--
include/uapi/linux/ip.h | 12 ++++++++++--
include/uapi/linux/ipv6.h | 12 ++++++++++--
net/core/flow_dissector.c | 10 ++++++----
net/ipv4/ip_output.c | 6 ++----
6 files changed, 42 insertions(+), 16 deletions(-)
@@ -81,8 +81,10 @@ struct flowi4 {#define flowi4_multipath_hash __fl_common.flowic_multipath_hash/* (saddr,daddr) must be grouped, same order as in IP header */-__be32saddr;-__be32daddr;+struct_group(addrs,+__be32saddr;+__be32daddr;+);unionflowi_uliuli;#define fl4_sport uli.ports.sport
@@ -163,8 +163,16 @@#if __UAPI_DEF_ETHHDRstructethhdr{-unsignedcharh_dest[ETH_ALEN];/* destination eth addr */-unsignedcharh_source[ETH_ALEN];/* source ether addr */+union{+struct{+unsignedcharh_dest[ETH_ALEN];/* destination eth addr */+unsignedcharh_source[ETH_ALEN];/* source ether addr */+};+struct{+unsignedcharh_dest[ETH_ALEN];/* destination eth addr */+unsignedcharh_source[ETH_ALEN];/* source ether addr */+}addrs;
A union of the same fields in the same structure in the same way?
Ah, because struct_group() can not be used here? Still feels odd to see
in a userspace-visible header.
quoted
+ };
__be16 h_proto; /* packet type ID field */
} __attribute__((packed));
#endif
From: Dan Carpenter <hidden> Date: 2021-07-28 07:36:39
On Tue, Jul 27, 2021 at 01:57:53PM -0700, Kees Cook wrote:
quoted hunk
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 <redacted>
---
include/net/ieee80211_radiotap.h | 24 ++++++++++++++++++++----
net/mac80211/rx.c | 2 +-
net/wireless/radiotap.c | 5 ++---
3 files changed, 23 insertions(+), 8 deletions(-)
@@ -39,10 +39,26 @@ struct ieee80211_radiotap_header {*/__le16it_len;-/**-*@it_present:(first)presentword-*/-__le32it_present;+union{+/**+*@it_present:(first)presentword+*/+__le32it_present;++struct{+/* The compiler makes it difficult to overlap+*aflex-arraywithanexistingsingleton,+*sowe'reforcedtoaddanemptynamed+*variablehere.+*/+struct{}__unused;++/**+*@bitmap:allpresencebitmaps+*/+__le32bitmap[];+};+};}__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:
From: David Sterba <hidden> Date: 2021-07-28 09:02:13
On Tue, Jul 27, 2021 at 01:57:52PM -0700, Kees Cook wrote:
quoted hunk
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 <redacted>
---
drivers/media/platform/omap3isp/ispstat.c | 5 +--
include/uapi/linux/omap3isp.h | 44 +++++++++++++++++------
2 files changed, 36 insertions(+), 13 deletions(-)
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
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.
From: David Sterba <hidden> Date: 2021-07-28 09:45:05
On Tue, Jul 27, 2021 at 01:58:38PM -0700, Kees Cook wrote:
quoted hunk
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Use memset_after() so memset() doesn't get confused about writing
beyond the destination member that is intended to be the starting point
of zeroing through the end of the struct.
Signed-off-by: Kees Cook <redacted>
---
fs/btrfs/root-tree.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
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;
};
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.
Co-developed-by: Keith Packard <redacted>
Signed-off-by: Keith Packard <redacted>
Signed-off-by: Kees Cook <redacted>
---
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 :(
Rasmus
At its core, FORTIFY_SOURCE uses the compiler's __builtin_object_size()
internal[0] to determine the available size at a target address based on
the compile-time known structure layout details. It operates in two
modes: outer bounds (0) and inner bounds (1). In mode 0, the size of the
enclosing structure is used. In mode 1, the size of the specific field
is used. For example:
struct object {
u16 scalar1; /* 2 bytes */
char array[6]; /* 6 bytes */
u64 scalar2; /* 8 bytes */
u32 scalar3; /* 4 bytes */
} instance;
__builtin_object_size(instance.array, 0) == 18, since the remaining size
of the enclosing structure starting from "array" is 18 bytes (6 + 8 + 4).
I think the compiler would usually end up making that struct size 24,
with 4 bytes of trailing padding (at least when alignof(u64) is 8). In
that case, does __builtin_object_size(instance.array, 0) actually
evaluate to 18, or to 22? A quick test on x86-64 suggests the latter, so
the memcpy(, , 20) would not be a violation.
Perhaps it's better to base the example on something which doesn't have
potential trailing padding - so either add another 4 byte member, or
also make scalar2 u32.
Rasmus
On Tue, Jul 27, 2021 at 01:58:53PM -0700, Kees Cook wrote:
quoted
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.
Add a flexible array member to mark the end of struct nlmsghdr, and
split the memcpy() to avoid false positive memcpy() warning:
memcpy: detected field-spanning write (size 32) of single field (size 16)
Signed-off-by: Kees Cook <redacted>
---
include/uapi/linux/netlink.h | 1 +
net/netlink/af_netlink.c | 4 +++-
2 files changed, 4 insertions(+), 1 deletion(-)
@@ -47,6 +47,7 @@ struct nlmsghdr {__u16nlmsg_flags;/* Additional flags */__u32nlmsg_seq;/* Sequence number */__u32nlmsg_pid;/* Sending process port ID */+__u8contents[];
Is this ok to change a public, userspace visable, structure?
At least it should keep using a nlmsg_ prefix for consistency and reduce
risk of collision with somebody having defined an object-like contents
macro. But there's no guarantees in any case, of course.
Rasmus
From: Stanislav Yakovlev <stas.yakovlev@gmail.com> Date: 2021-07-28 18:56:10
On 28/07/2021, Kees Cook [off-list ref] wrote:
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 <redacted>
---
drivers/net/wireless/intel/ipw2x00/libipw.h | 12 +++++++-----
drivers/net/wireless/intel/ipw2x00/libipw_rx.c | 8 ++++----
2 files changed, 11 insertions(+), 9 deletions(-)
Acked-by: Stanislav Yakovlev <stas.yakovlev@gmail.com>
Looks fine, thanks!
Stanislav.
A union of the same fields in the same structure in the same way?
Ah, because struct_group() can not be used here? Still feels odd to see
in a userspace-visible header.
Yeah, there is some inconsistency here. I will clean this up for v2.
Is there a place we can put kernel-specific macros for use in UAPI
headers? (I need to figure out where things like __kernel_size_t get
defined...)
--
Kees Cook
On Wed, Jul 28, 2021 at 10:35:56AM +0300, Dan Carpenter wrote:
On Tue, Jul 27, 2021 at 01:57:53PM -0700, Kees Cook wrote:
quoted
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 <redacted>
---
include/net/ieee80211_radiotap.h | 24 ++++++++++++++++++++----
net/mac80211/rx.c | 2 +-
net/wireless/radiotap.c | 5 ++---
3 files changed, 23 insertions(+), 8 deletions(-)
@@ -39,10 +39,26 @@ struct ieee80211_radiotap_header {*/__le16it_len;-/**-*@it_present:(first)presentword-*/-__le32it_present;+union{+/**+*@it_present:(first)presentword+*/+__le32it_present;++struct{+/* The compiler makes it difficult to overlap+*aflex-arraywithanexistingsingleton,+*sowe'reforcedtoaddanemptynamed+*variablehere.+*/+struct{}__unused;++/**+*@bitmap:allpresencebitmaps+*/+__le32bitmap[];+};+};}__packed;
This patch is so confusing...
Yeah, I agree. I tried a few ways, and was unhappy with all of them. :P
quoted hunk
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:
Hm, yes, I can try this. I attempted something similar without the
"only a struct" part; I was trying to avoid the identifier churn, but I
guess seeing it again, it's not _that_ bad. :P
Hm, interesting way to avoid angering the compiler during the later
it_present++ updates. This is subtle ... a passer-by may not understand
why this isn't just "it_present = &rthdr->data.it_present".
I think this is okay with a comment added. I'll give this a spin.
Thanks!
-Kees
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.
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.
From: Bart Van Assche <bvanassche@acm.org> Date: 2021-07-28 21:46:02
On 7/27/21 1:58 PM, Kees Cook wrote:
quoted hunk
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Add a struct_group() for the algs so that memset() can correctly reason
about the size.
Signed-off-by: Kees Cook <redacted>
---
drivers/block/drbd/drbd_main.c | 3 ++-
drivers/block/drbd/drbd_protocol.h | 6 ++++--
drivers/block/drbd/drbd_receiver.c | 3 ++-
3 files changed, 8 insertions(+), 4 deletions(-)
@@ -3921,7 +3921,8 @@ static int receive_SyncParam(struct drbd_connection *connection, struct packet_i/* initialize verify_alg and csums_alg */p=pi->data;-memset(p->verify_alg,0,2*SHARED_SECRET_MAX);+BUILD_BUG_ON(sizeof(p->algs)!=2*SHARED_SECRET_MAX);+memset(&p->algs,0,sizeof(p->algs));
Using struct_group() introduces complexity. Has it been considered not
to modify struct p_rs_param_95 and instead to use two memset() calls
instead of one (one memset() call per member)?
Thanks,
Bart.
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.
Ah, good catch! I think just using BIT() is the right replacement here,
yes? I suppose that should be a separate patch.
--
Kees Cook
On Wed, Jul 28, 2021 at 11:42:15AM +0200, David Sterba wrote:
On Tue, Jul 27, 2021 at 01:58:38PM -0700, Kees Cook wrote:
quoted
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Use memset_after() so memset() doesn't get confused about writing
beyond the destination member that is intended to be the starting point
of zeroing through the end of the struct.
Signed-off-by: Kees Cook <redacted>
---
fs/btrfs/root-tree.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
Please add
/* Clear all members from generation_v2 onwards */
quoted
+ memset_after(item, 0, level);
Perhaps there should be another helper memset_starting()? That would
make these cases a bit more self-documenting.
+ memset_starting(item, 0, generation_v2);
On Wed, Jul 28, 2021 at 12:54:18PM +0200, Rasmus Villemoes wrote:
On 27/07/2021 22.57, Kees Cook wrote:
quoted
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;
};
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. ;)
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.
Ah! Perfect, thank you. I totally forgot I could do it that way.
quoted
Co-developed-by: Keith Packard <redacted>
Signed-off-by: Keith Packard <redacted>
Signed-off-by: Kees Cook <redacted>
---
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 :(
My instinct is to make these kinds of helpers "always available" (like
sizeof_field(), etc), but I have no strong opinion on where it should
live. If the consensus is to move it, I certainly can! :)
-Kees
--
Kees Cook
On Wed, Jul 28, 2021 at 10:35:56AM +0300, Dan Carpenter wrote:
On Tue, Jul 27, 2021 at 01:57:53PM -0700, Kees Cook wrote:
quoted
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 <redacted>
---
include/net/ieee80211_radiotap.h | 24 ++++++++++++++++++++----
net/mac80211/rx.c | 2 +-
net/wireless/radiotap.c | 5 ++---
3 files changed, 23 insertions(+), 8 deletions(-)
@@ -39,10 +39,26 @@ struct ieee80211_radiotap_header {*/__le16it_len;-/**-*@it_present:(first)presentword-*/-__le32it_present;+union{+/**+*@it_present:(first)presentword+*/+__le32it_present;++struct{+/* The compiler makes it difficult to overlap+*aflex-arraywithanexistingsingleton,+*sowe'reforcedtoaddanemptynamed+*variablehere.+*/+struct{}__unused;++/**+*@bitmap:allpresencebitmaps+*/+__le32bitmap[];+};+};}__packed;
This patch is so confusing...
Right, unfortunately your patch doesn't work under the strict memcpy().
:(
Here are the constraints I navigated to come to the original patch I
sent:
* I need to directly reference a flexible array for the it_present
pointer because pos is based on it, and the compiler thinks pos
walks off the end of the struct:
In function 'fortify_memcpy_chk',
inlined from 'ieee80211_add_rx_radiotap_header' at net/mac80211/rx.c:652:3:
./include/linux/fortify-string.h:285:4: warning: call to '__write_overflow_field' declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Wattribute-warning]
285 | __write_overflow_field();
| ^~~~~~~~~~~~~~~~~~~~~~~~
* It's churn/fragile to change the sizeof(), so I can't just do:
- __le32 it_present;
+ __le32 it_bitmap[];
* I want to use a union:
- __le32 it_present;
+ union {
+ __le32 it_present;
+ __le32 it_bitmap[];
+ };
* ... but I can't actually use a union because of compiler constraints
on flexible array members:
./include/net/ieee80211_radiotap.h:50:10: error: flexible array member in union
50 | __le32 it_optional[];
| ^~~~~~~~~~~
* So I came to the horrible thing I original sent. :P
If I could escape the __le32 *it_present incrementing, I could use a
simple change:
__le32 it_present;
+ __le32 it_optional[];
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.
Hm, docs seem to indicate that the packet format is multiples of u32?
*shrug*
Hmpf.
-Kees
--
Kees Cook
On Wed, Jul 28, 2021 at 10:35:56AM +0300, Dan Carpenter wrote:
quoted hunk
On Tue, Jul 27, 2021 at 01:57:53PM -0700, Kees Cook wrote:
quoted
[...]
- /**
- * @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;
+
+ /**
+ * @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:
@@ -359,7 +359,13 @@ ieee80211_add_rx_radiotap_header(struct ieee80211_local *local,put_unaligned_le32(it_present_val,it_present);-pos=(void*)(it_present+1);+/*+*Thisreferencesthroughanoffsetintoit_optional[]rather+*thanviait_presentotherwiselaterusesofposwillcause+*thecompilertothinkwehavewalkedpasttheendofthe+*structmember.+*/+pos=(void*)&rthdr->it_optional[it_present-rthdr->it_optional];/* the order of the following fields is important */
A union of the same fields in the same structure in the same way?
Ah, because struct_group() can not be used here? Still feels odd to see
in a userspace-visible header.
Yeah, there is some inconsistency here. I will clean this up for v2.
Is there a place we can put kernel-specific macros for use in UAPI
headers? (I need to figure out where things like __kernel_size_t get
defined...)
How about using two memset() calls to clear h_dest[] and h_source[]
instead of modifying the uapi header?
Thanks,
Bart.
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
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...
From: David Sterba <hidden> Date: 2021-07-29 10:36:27
On Wed, Jul 28, 2021 at 02:56:31PM -0700, Kees Cook wrote:
On Wed, Jul 28, 2021 at 11:42:15AM +0200, David Sterba wrote:
quoted
On Tue, Jul 27, 2021 at 01:58:38PM -0700, Kees Cook wrote:
quoted
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Use memset_after() so memset() doesn't get confused about writing
beyond the destination member that is intended to be the starting point
of zeroing through the end of the struct.
Signed-off-by: Kees Cook <redacted>
---
fs/btrfs/root-tree.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
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.
Ah, good catch! I think just using BIT() is the right replacement here,
yes? I suppose that should be a separate patch.
From: Daniel Vetter <hidden> Date: 2021-07-29 12:11:39
On Wed, Jul 28, 2021 at 07:56:40AM +0200, Greg Kroah-Hartman wrote:
On Tue, Jul 27, 2021 at 01:58:16PM -0700, Kees Cook wrote:
quoted
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() in struct drm32_mga_init around members chipset, sgram,
maccess, fb_cpp, front_offset, front_pitch, back_offset, back_pitch,
depth_cpp, depth_offset, depth_pitch, texture_offset, and texture_size,
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 chipset.
"pahole" shows no size nor member offset changes to struct drm32_mga_init.
"objdump -d" shows no meaningful object code changes (i.e. only source
line number induced differences and optimizations).
Note that since this includes a UAPI header, struct_group() has been
explicitly redefined local to the header.
Signed-off-by: Kees Cook <redacted>
---
drivers/gpu/drm/mga/mga_ioc32.c | 30 ++++++++++++++------------
include/uapi/drm/mga_drm.h | 37 ++++++++++++++++++++++++---------
2 files changed, 44 insertions(+), 23 deletions(-)
Why can you use __struct_group in this uapi header, but not the
networking one?
If there's others, maybe we can stuff the uapi __struct_group into
linux/types.h where all the other __ uapi types hang out?
Anyway mga is very dead, I don't anyone cares.
Acked-by: Daniel Vetter <redacted>
I'm assuming this goes in through a topic pull from you?
I'll leave the drm/amd one to figure out between you and Alex.
-Daniel
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-07-29 18:58:58
On Tue, 27 Jul 2021 13:58:45 -0700 Kees Cook wrote:
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Add struct_group() to mark region of struct rt6_info that should be
initialized to zero.
@@ -205,20 +205,22 @@ struct fib6_info {structrt6_info{structdst_entrydst;-structfib6_info__rcu*from;-intsernum;--structrt6keyrt6i_dst;-structrt6keyrt6i_src;-structin6_addrrt6i_gateway;-structinet6_dev*rt6i_idev;-u32rt6i_flags;--structlist_headrt6i_uncached;-structuncached_list*rt6i_uncached_list;--/* more non-fragment space at head required */-unsignedshortrt6i_nfheader_len;+struct_group(init,+structfib6_info__rcu*from;+intsernum;++structrt6keyrt6i_dst;+structrt6keyrt6i_src;+structin6_addrrt6i_gateway;+structinet6_dev*rt6i_idev;+u32rt6i_flags;++structlist_headrt6i_uncached;+structuncached_list*rt6i_uncached_list;++/* more non-fragment space at head required */+unsignedshortrt6i_nfheader_len;+);};structfib6_result{
On Wed, Jul 28, 2021 at 01:24:01PM +0200, Rasmus Villemoes wrote:
On 28/07/2021 07.49, Greg Kroah-Hartman wrote:
quoted
On Tue, Jul 27, 2021 at 01:58:53PM -0700, Kees Cook wrote:
quoted
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.
Add a flexible array member to mark the end of struct nlmsghdr, and
split the memcpy() to avoid false positive memcpy() warning:
memcpy: detected field-spanning write (size 32) of single field (size 16)
Signed-off-by: Kees Cook <redacted>
---
include/uapi/linux/netlink.h | 1 +
net/netlink/af_netlink.c | 4 +++-
2 files changed, 4 insertions(+), 1 deletion(-)
@@ -47,6 +47,7 @@ struct nlmsghdr {__u16nlmsg_flags;/* Additional flags */__u32nlmsg_seq;/* Sequence number */__u32nlmsg_pid;/* Sending process port ID */+__u8contents[];
Is this ok to change a public, userspace visable, structure?
At least it should keep using a nlmsg_ prefix for consistency and reduce
risk of collision with somebody having defined an object-like contents
macro. But there's no guarantees in any case, of course.
Ah, good call. I've adjusted this and added a comment.
Thanks!
-Kees
--
Kees Cook
On Wed, Jul 28, 2021 at 07:49:46AM +0200, Greg Kroah-Hartman wrote:
On Tue, Jul 27, 2021 at 01:58:53PM -0700, Kees Cook wrote:
quoted
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.
Add a flexible array member to mark the end of struct nlmsghdr, and
split the memcpy() to avoid false positive memcpy() warning:
memcpy: detected field-spanning write (size 32) of single field (size 16)
Signed-off-by: Kees Cook <redacted>
---
include/uapi/linux/netlink.h | 1 +
net/netlink/af_netlink.c | 4 +++-
2 files changed, 4 insertions(+), 1 deletion(-)
@@ -47,6 +47,7 @@ struct nlmsghdr {__u16nlmsg_flags;/* Additional flags */__u32nlmsg_seq;/* Sequence number */__u32nlmsg_pid;/* Sending process port ID */+__u8contents[];
Is this ok to change a public, userspace visable, structure?
Nothing breaks?
It really shouldn't break anything. Adding a flex array doesn't change
the size. And with Rasmus's suggestion (naming it "nlmsg_content") it
should be safe against weird global macro collisions, etc.
--
Kees Cook
From: Alex Deucher <hidden> Date: 2021-07-30 02:07:53
On Tue, Jul 27, 2021 at 5:17 PM Kees Cook [off-list ref] 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.
Use struct_group() in structs:
struct atom_smc_dpm_info_v4_5
struct atom_smc_dpm_info_v4_6
struct atom_smc_dpm_info_v4_7
struct atom_smc_dpm_info_v4_10
PPTable_t
so the grouped members 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
the first member.
"pahole" shows no size nor member offset changes to any structs.
"objdump -d" shows no object code changes.
These headers represent interfaces with firmware running on
microcontrollers, so if the sizes or offsets change that could cause a
problem. That doesn't seem to be the case, but something to keep in
mind. Patch is:
Acked-by: Alex Deucher <alexander.deucher@amd.com>
Feel free to take this through whatever tree makes sense.
Alex
@@ -2159,7 +2160,7 @@ struct atom_smc_dpm_info_v4_5uint32_tMvddRatio;// This is used for MVDD Vid workaround. It has 16 fractional bits (Q16.16)uint32_tBoardReserved[9];-+);};structatom_smc_dpm_info_v4_6
@@ -2168,6 +2169,7 @@ struct atom_smc_dpm_info_v4_6// section: board parametersuint32_ti2c_padding[3];// old i2c control are moved to new area+struct_group(dpm_info,uint16_tmaxvoltagestepgfx;// in mv(q2) max voltage step that smu will request. multiple steps are taken if voltage change exceeds this value.uint16_tmaxvoltagestepsoc;// in mv(q2) max voltage step that smu will request. multiple steps are taken if voltage change exceeds this value.
@@ -643,6 +643,7 @@ typedef struct {// SECTION: BOARD PARAMETERS// SVI2 Board Parameters+struct_group(v4_6,uint16_tMaxVoltageStepGfx;// In mV(Q2) Max voltage step that SMU will request. Multiple steps are taken if voltage change exceeds this value.uint16_tMaxVoltageStepSoc;// In mV(Q2) Max voltage step that SMU will request. Multiple steps are taken if voltage change exceeds this value.
@@ -728,10 +729,10 @@ typedef struct {uint32_tBoardVoltageCoeffB;// decode by /1000uint32_tBoardReserved[7];+);// Padding for MMHUB - do not modify thisuint32_tMmHubPadding[8];// SMU internal use-}PPTable_t;typedefstruct{
On Wed, Jul 28, 2021 at 02:45:55PM -0700, Bart Van Assche wrote:
On 7/27/21 1:58 PM, Kees Cook wrote:
quoted
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Add a struct_group() for the algs so that memset() can correctly reason
about the size.
Signed-off-by: Kees Cook <redacted>
---
drivers/block/drbd/drbd_main.c | 3 ++-
drivers/block/drbd/drbd_protocol.h | 6 ++++--
drivers/block/drbd/drbd_receiver.c | 3 ++-
3 files changed, 8 insertions(+), 4 deletions(-)
@@ -3921,7 +3921,8 @@ static int receive_SyncParam(struct drbd_connection *connection, struct packet_i/* initialize verify_alg and csums_alg */p=pi->data;-memset(p->verify_alg,0,2*SHARED_SECRET_MAX);+BUILD_BUG_ON(sizeof(p->algs)!=2*SHARED_SECRET_MAX);+memset(&p->algs,0,sizeof(p->algs));
Using struct_group() introduces complexity. Has it been considered not to
modify struct p_rs_param_95 and instead to use two memset() calls instead of
one (one memset() call per member)?
I went this direction because using two memset()s (or memcpy()s in other
patches) changes the machine code. It's not much of a change, but it
seems easier to justify "no binary changes" via the use of struct_group().
If splitting the memset() is preferred, I can totally do that instead.
:)
-Kees
--
Kees Cook
On Wed, Jul 28, 2021 at 01:19:59PM +0200, Rasmus Villemoes wrote:
On 27/07/2021 22.58, Kees Cook wrote:
quoted
At its core, FORTIFY_SOURCE uses the compiler's __builtin_object_size()
internal[0] to determine the available size at a target address based on
the compile-time known structure layout details. It operates in two
modes: outer bounds (0) and inner bounds (1). In mode 0, the size of the
enclosing structure is used. In mode 1, the size of the specific field
is used. For example:
struct object {
u16 scalar1; /* 2 bytes */
char array[6]; /* 6 bytes */
u64 scalar2; /* 8 bytes */
u32 scalar3; /* 4 bytes */
} instance;
__builtin_object_size(instance.array, 0) == 18, since the remaining size
of the enclosing structure starting from "array" is 18 bytes (6 + 8 + 4).
I think the compiler would usually end up making that struct size 24,
with 4 bytes of trailing padding (at least when alignof(u64) is 8). In
that case, does __builtin_object_size(instance.array, 0) actually
evaluate to 18, or to 22? A quick test on x86-64 suggests the latter, so
the memcpy(, , 20) would not be a violation.
Perhaps it's better to base the example on something which doesn't have
potential trailing padding - so either add another 4 byte member, or
also make scalar2 u32.
Yup, totally right. Thanks! I've fixed the example now for v2.
--
Kees Cook
From: Bart Van Assche <bvanassche@acm.org> Date: 2021-07-30 02:57:54
On 7/29/21 7:31 PM, Kees Cook wrote:
On Wed, Jul 28, 2021 at 02:45:55PM -0700, Bart Van Assche wrote:
quoted
On 7/27/21 1:58 PM, Kees Cook wrote:
quoted
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Add a struct_group() for the algs so that memset() can correctly reason
about the size.
Signed-off-by: Kees Cook <redacted>
---
drivers/block/drbd/drbd_main.c | 3 ++-
drivers/block/drbd/drbd_protocol.h | 6 ++++--
drivers/block/drbd/drbd_receiver.c | 3 ++-
3 files changed, 8 insertions(+), 4 deletions(-)
@@ -3921,7 +3921,8 @@ static int receive_SyncParam(struct drbd_connection *connection, struct packet_i/* initialize verify_alg and csums_alg */p=pi->data;-memset(p->verify_alg,0,2*SHARED_SECRET_MAX);+BUILD_BUG_ON(sizeof(p->algs)!=2*SHARED_SECRET_MAX);+memset(&p->algs,0,sizeof(p->algs));
Using struct_group() introduces complexity. Has it been considered not to
modify struct p_rs_param_95 and instead to use two memset() calls instead of
one (one memset() call per member)?
I went this direction because using two memset()s (or memcpy()s in other
patches) changes the machine code. It's not much of a change, but it
seems easier to justify "no binary changes" via the use of struct_group().
If splitting the memset() is preferred, I can totally do that instead.
:)
I don't have a strong opinion about this. Lars, do you want to comment
on this patch?
Thanks,
Bart.
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...
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
--
Kees Cook
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.
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.
3945ff37d2f4 ("linux/bits.h: Extract common header for vDSO") moved it
there from linux/bits.h, and linux/bits.h now includes vdso/bits.h, so
it is still ever-present. :)
--
Kees Cook
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...
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
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.
From: Lars Ellenberg <lars.ellenberg@linbit.com> Date: 2021-07-30 09:26:06
On Thu, Jul 29, 2021 at 07:57:47PM -0700, Bart Van Assche wrote:
On 7/29/21 7:31 PM, Kees Cook wrote:
quoted
On Wed, Jul 28, 2021 at 02:45:55PM -0700, Bart Van Assche wrote:
quoted
On 7/27/21 1:58 PM, Kees Cook wrote:
quoted
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Add a struct_group() for the algs so that memset() can correctly reason
about the size.
Signed-off-by: Kees Cook <redacted>
---
drivers/block/drbd/drbd_main.c | 3 ++-
drivers/block/drbd/drbd_protocol.h | 6 ++++--
drivers/block/drbd/drbd_receiver.c | 3 ++-
3 files changed, 8 insertions(+), 4 deletions(-)
@@ -3921,7 +3921,8 @@ static int receive_SyncParam(struct drbd_connection *connection, struct packet_i/* initialize verify_alg and csums_alg */p=pi->data;-memset(p->verify_alg,0,2*SHARED_SECRET_MAX);+BUILD_BUG_ON(sizeof(p->algs)!=2*SHARED_SECRET_MAX);+memset(&p->algs,0,sizeof(p->algs));
Using struct_group() introduces complexity. Has it been considered not to
modify struct p_rs_param_95 and instead to use two memset() calls instead of
one (one memset() call per member)?
I went this direction because using two memset()s (or memcpy()s in other
patches) changes the machine code. It's not much of a change, but it
seems easier to justify "no binary changes" via the use of struct_group().
If splitting the memset() is preferred, I can totally do that instead.
:)
I don't have a strong opinion about this. Lars, do you want to comment
on this patch?
Fine either way. "no binary changes" sounds good ;-)
Thanks,
Lars
From: Nick Desaulniers <ndesaulniers@google.com> Date: 2021-07-30 15:32:26
On Thu, Jul 29, 2021 at 7:31 PM Kees Cook [off-list ref] wrote:
On Wed, Jul 28, 2021 at 02:45:55PM -0700, Bart Van Assche wrote:
quoted
On 7/27/21 1:58 PM, Kees Cook wrote:
quoted
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Add a struct_group() for the algs so that memset() can correctly reason
about the size.
Signed-off-by: Kees Cook <redacted>
---
drivers/block/drbd/drbd_main.c | 3 ++-
drivers/block/drbd/drbd_protocol.h | 6 ++++--
drivers/block/drbd/drbd_receiver.c | 3 ++-
3 files changed, 8 insertions(+), 4 deletions(-)
@@ -3921,7 +3921,8 @@ static int receive_SyncParam(struct drbd_connection *connection, struct packet_i/* initialize verify_alg and csums_alg */p=pi->data;-memset(p->verify_alg,0,2*SHARED_SECRET_MAX);+BUILD_BUG_ON(sizeof(p->algs)!=2*SHARED_SECRET_MAX);+memset(&p->algs,0,sizeof(p->algs));
Using struct_group() introduces complexity. Has it been considered not to
modify struct p_rs_param_95 and instead to use two memset() calls instead of
one (one memset() call per member)?
I went this direction because using two memset()s (or memcpy()s in other
patches) changes the machine code. It's not much of a change, but it
seems easier to justify "no binary changes" via the use of struct_group().
If splitting the memset() is preferred, I can totally do that instead.
:)
I'm not sure that compilers can fold memsets of adjacent members. It
might not matter, but you could wrap these members in a _named_ struct
then simply use assignment for optimal codegen.
--
Thanks,
~Nick Desaulniers
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:
quoted
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.)
--
Kees Cook
From: Williams, Dan J <hidden> Date: 2021-07-30 22:19:58
On Wed, 2021-07-28 at 14:59 -0700, Kees Cook wrote:
On Wed, Jul 28, 2021 at 12:54:18PM +0200, Rasmus Villemoes wrote:
quoted
On 27/07/2021 22.57, Kees Cook wrote:
quoted
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;
};
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
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.
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/:
@@ -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-CommonsetofCXLComponentregisterblockbasepointers*@hdm_decoder:CXL2.08.2.5.12CXLHDMDecoderCapabilityStructure-*/-structcxl_component_regs{-CXL_COMPONENT_REGS();-};--/* See note for 'struct cxl_regs' for the rationale of this organization */-/*-*CXL_DEVICE_REGS-CommonsetofCXLDeviceregisterblockbasepointers*@status:CXL2.08.2.8.3DeviceStatusRegisters*@mbox:CXL2.08.2.8.4MailboxRegisters*@memdev:CXL2.08.2.8.5MemoryDeviceRegisters*/-structcxl_device_regs{-CXL_DEVICE_REGS();-};--/*-*Note,theanonymousunionorganizationallowsforper-*register-block-typehelperroutines,withoutrequiringblock-type-*agnosticcodetoincludetheprefix.-*/structcxl_regs{-union{-struct{-CXL_COMPONENT_REGS();-};-structcxl_component_regscomponent;-};-union{-struct{-CXL_DEVICE_REGS();-};-structcxl_device_regsdevice_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;+);};structcxl_reg_map{
On Fri, Jul 30, 2021 at 10:19:20PM +0000, Williams, Dan J wrote:
On Wed, 2021-07-28 at 14:59 -0700, Kees Cook wrote:
quoted
On Wed, Jul 28, 2021 at 12:54:18PM +0200, Rasmus Villemoes wrote:
quoted
On 27/07/2021 22.57, Kees Cook wrote:
quoted
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;
};
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
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.
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?
Oh cool! Yeah, totally I can expand it. Thanks for the suggestion!
quoted hunk
Here is a rough idea of the cleanup it would induce in drivers/cxl/:
@@ -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-CommonsetofCXLComponentregisterblockbasepointers*@hdm_decoder:CXL2.08.2.5.12CXLHDMDecoderCapabilityStructure-*/-structcxl_component_regs{-CXL_COMPONENT_REGS();-};--/* See note for 'struct cxl_regs' for the rationale of this organization */-/*-*CXL_DEVICE_REGS-CommonsetofCXLDeviceregisterblockbasepointers*@status:CXL2.08.2.8.3DeviceStatusRegisters*@mbox:CXL2.08.2.8.4MailboxRegisters*@memdev:CXL2.08.2.8.5MemoryDeviceRegisters*/-structcxl_device_regs{-CXL_DEVICE_REGS();-};--/*-*Note,theanonymousunionorganizationallowsforper-*register-block-typehelperroutines,withoutrequiringblock-type-*agnosticcodetoincludetheprefix.-*/structcxl_regs{-union{-struct{-CXL_COMPONENT_REGS();-};-structcxl_component_regscomponent;-};-union{-struct{-CXL_DEVICE_REGS();-};-structcxl_device_regsdevice_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;+);};structcxl_reg_map{
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...)
--
Kees Cook
On Thu, Jul 29, 2021 at 02:11:27PM +0200, Daniel Vetter wrote:
On Wed, Jul 28, 2021 at 07:56:40AM +0200, Greg Kroah-Hartman wrote:
quoted
On Tue, Jul 27, 2021 at 01:58:16PM -0700, Kees Cook wrote:
quoted
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() in struct drm32_mga_init around members chipset, sgram,
maccess, fb_cpp, front_offset, front_pitch, back_offset, back_pitch,
depth_cpp, depth_offset, depth_pitch, texture_offset, and texture_size,
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 chipset.
"pahole" shows no size nor member offset changes to struct drm32_mga_init.
"objdump -d" shows no meaningful object code changes (i.e. only source
line number induced differences and optimizations).
Note that since this includes a UAPI header, struct_group() has been
explicitly redefined local to the header.
[...]
Why can you use __struct_group in this uapi header, but not the
networking one?
If there's others, maybe we can stuff the uapi __struct_group into
linux/types.h where all the other __ uapi types hang out?
Ah yeah; it looks like include/uapi/linux/stddef.h is the place for it.
Anyway mga is very dead, I don't anyone cares.
Acked-by: Daniel Vetter <redacted>
I'm assuming this goes in through a topic pull from you?
Thanks! Yeah, my intention is to carry this as topic branch for Linus.
-Kees
--
Kees Cook
On Thu, Jul 29, 2021 at 11:58:50AM -0700, Jakub Kicinski wrote:
On Tue, 27 Jul 2021 13:58:45 -0700 Kees Cook wrote:
quoted
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Add struct_group() to mark region of struct rt6_info that should be
initialized to zero.
On Thu, Jul 29, 2021 at 12:33:37PM +0200, David Sterba wrote:
On Wed, Jul 28, 2021 at 02:56:31PM -0700, Kees Cook wrote:
quoted
On Wed, Jul 28, 2021 at 11:42:15AM +0200, David Sterba wrote:
quoted
On Tue, Jul 27, 2021 at 01:58:38PM -0700, Kees Cook wrote:
quoted
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Use memset_after() so memset() doesn't get confused about writing
beyond the destination member that is intended to be the starting point
of zeroing through the end of the struct.
Signed-off-by: Kees Cook <redacted>
---
fs/btrfs/root-tree.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
Please add
/* Clear all members from generation_v2 onwards */
quoted
+ memset_after(item, 0, level);
Perhaps there should be another helper memset_starting()? That would
make these cases a bit more self-documenting.
That would be better, yes.
quoted
+ memset_starting(item, 0, generation_v2);
memset_from?
For v2, I bikeshed this to "memset_startat" since "from" is semantically
close to "source" which I thought might be confusing. (I, too, did not
like "starting".) :)
Can I make "bikeshed" a verb? :P
--
Kees Cook
On Tue, Jul 27, 2021 at 01:58:30PM -0700, Kees Cook wrote:
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Use memset_after() so memset() doesn't get confused about writing
beyond the destination member that is intended to be the starting point
of zeroing through the end of the struct.
Note that the common helper, ieee80211_tx_info_clear_status(), does NOT
clear ack_signal, but the open-coded versions do. All three perform
checks that the ack_signal position hasn't changed, though.
Quick ping on this question: there is a mismatch between the common
helper and the other places that do this. Is there a bug here?
quoted hunk
Signed-off-by: Kees Cook <redacted>
---
Should these each be clearing the same region? Because they're currently not.
---
drivers/net/wireless/ath/carl9170/tx.c | 4 +---
drivers/net/wireless/intersil/p54/txrx.c | 4 +---
include/net/mac80211.h | 4 +---
3 files changed, 3 insertions(+), 9 deletions(-)
On Tue, Jul 27, 2021 at 01:58:33PM -0700, Kees Cook wrote:
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Use memset_after() so memset() doesn't get confused about writing
beyond the destination member that is intended to be the starting point
of zeroing through the end of the struct.
Signed-off-by: Kees Cook <redacted>
---
The old code seems to be doing the wrong thing: starting from not the
first member, but sized for the whole struct. Which is correct?
Quick ping on this question.
The old code seems to be doing the wrong thing: it starts from the second
member and writes beyond int_info, clobbering qede_lock:
struct qede_dev {
...
struct qed_int_info int_info;
/* Smaller private variant of the RTNL lock */
struct mutex qede_lock;
...
struct qed_int_info {
struct msix_entry *msix;
u8 msix_cnt;
/* This should be updated by the protocol driver */
u8 used_cnt;
};
Should this also clear the "msix" member, or should this not write
beyond int_info? This patch does the latter.
-Kees
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2021-08-05 11:37:07
Kees Cook [off-list ref] writes:
quoted hunk
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Instead of writing across a field boundary with memset(), move the call
to just the array, and an explicit zeroing of the prior field.
Signed-off-by: Kees Cook <redacted>
---
drivers/macintosh/smu.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Please add
/* Clear all members from generation_v2 onwards */
quoted
+ memset_after(item, 0, level);
Perhaps there should be another helper memset_starting()? That would
make these cases a bit more self-documenting.
That would be better, yes.
quoted
+ memset_starting(item, 0, generation_v2);
memset_from?
For v2, I bikeshed this to "memset_startat" since "from" is semantically
close to "source" which I thought might be confusing. (I, too, did not
like "starting".) :)
From: Johannes Berg <johannes@sipsolutions.net> Date: 2021-08-13 07:40:26
On Sat, 2021-07-31 at 08:55 -0700, Kees Cook wrote:
On Tue, Jul 27, 2021 at 01:58:30PM -0700, Kees Cook wrote:
quoted
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Use memset_after() so memset() doesn't get confused about writing
beyond the destination member that is intended to be the starting point
of zeroing through the end of the struct.
Note that the common helper, ieee80211_tx_info_clear_status(), does NOT
clear ack_signal, but the open-coded versions do. All three perform
checks that the ack_signal position hasn't changed, though.
Quick ping on this question: there is a mismatch between the common
helper and the other places that do this. Is there a bug here?
Yes.
The common helper should also clear ack_signal, but that was broken by
commit e3e1a0bcb3f1 ("mac80211: reduce IEEE80211_TX_MAX_RATES"), because
that commit changed the order of the fields and updated carl9170 and p54
properly but not the common helper...
It doesn't actually matter much because ack_signal is normally filled in
afterwards, and even if it isn't, it's just for statistics.
The correct thing to do here would be to
memset_after(&info->status, 0, rates);
johannes
FWIW, I think we should also remove the BUILD_BUG_ON() now in all the
places - that was meant to give people a hint to update if some field
ordering etc. changed, but now that it's "after rates" this is no longer
necessary.
johannes
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.
quoted hunk
+++ b/net/wireless/lib80211_crypt_ccmp.c
@@ -136,7 +136,8 @@ static int ccmp_init_iv_and_aad(const struct ieee80211_hdr *hdr,
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 ...
johannes
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.
quoted
+++ b/net/wireless/lib80211_crypt_ccmp.c
@@ -136,7 +136,8 @@ static int ccmp_init_iv_and_aad(const struct ieee80211_hdr *hdr,
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?
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 which case I'd probably prefer to do this separately from the staging
drivers ...
Agreed. Sorry for the noise on that part. I will double-check the other
patches.
--
Kees Cook
On Fri, Aug 13, 2021 at 09:40:07AM +0200, Johannes Berg wrote:
On Sat, 2021-07-31 at 08:55 -0700, Kees Cook wrote:
quoted
On Tue, Jul 27, 2021 at 01:58:30PM -0700, Kees Cook wrote:
quoted
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Use memset_after() so memset() doesn't get confused about writing
beyond the destination member that is intended to be the starting point
of zeroing through the end of the struct.
Note that the common helper, ieee80211_tx_info_clear_status(), does NOT
clear ack_signal, but the open-coded versions do. All three perform
checks that the ack_signal position hasn't changed, though.
Quick ping on this question: there is a mismatch between the common
helper and the other places that do this. Is there a bug here?
Yes.
The common helper should also clear ack_signal, but that was broken by
commit e3e1a0bcb3f1 ("mac80211: reduce IEEE80211_TX_MAX_RATES"), because
that commit changed the order of the fields and updated carl9170 and p54
properly but not the common helper...
It looks like p54 actually uses the rates, which is why it does this
manually. I can't see why carl9170 does this manually, though.
It doesn't actually matter much because ack_signal is normally filled in
afterwards, and even if it isn't, it's just for statistics.
The correct thing to do here would be to
memset_after(&info->status, 0, rates);
Sounds good; I will adjust these (and drop the BULID_BUG_ONs, as you
suggest in the next email).
Thanks!
-Kees
--
Kees Cook