| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZzK-n_C2yl8mW2Tz@kspp>
Date: Mon, 11 Nov 2024 20:34:07 -0600
From: "Gustavo A. R. Silva" <gustavoars@...nel.org>
To: "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Simon Horman <horms@...nel.org>, David Ahern <dsahern@...nel.org>,
Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
"Gustavo A. R. Silva" <gustavoars@...nel.org>,
linux-hardening@...r.kernel.org, Kees Cook <kees@...nel.org>
Subject: [PATCH RFC][next] net: inet_sock.h: Avoid thousands of
-Wflex-array-member-not-at-end warnings
-Wflex-array-member-not-at-end was introduced in GCC-14, and we are
getting ready to enable it, globally
So, in order to avoid ending up with a flexible-array member in the
middle of another struct, we use the `struct_group_tagged()` helper
to create a new tagged `struct ip_options_hdr`. This structure groups
together all the members of the flexible `struct ip_options` except
the flexible array. As a result, the array is effectively separated
from the rest of the members without modifying the memory layout of
the flexible structure.
However, the pattern that triggers the warnings addressed by this patch
differs from the usual case. Typically, the flexible structure is
immediately followed by another object in the composite structure. Here,
however, we have a well-defined composite structure with a flexible
structure right at its end, which is a correct code construct and has
no issues on its own. An instance of this composite structure
(`struct ip_options_rcu`) is then used within another structure
(`struct ip_options_data`) right before a fixed-size array (`char
data[40]`), as below:
/* Modified flexible structure with struct_group_tagged() */
struct ip_options {
struct_group_tagged(ip_options_hdr, __hdr,
...
);
unsigned char __data[];
};
/* Correct composite struct with flex struct at the end. */
struct ip_options_rcu {
struct rcu_head rcu;
struct ip_options opt; /* flexible structure --this is fine. */
};
struct ip_options_data {
struct ip_options_rcu opt; /* Conflicting object causing tons
* of -Wfamnae warnings --it ends in
* flexible-array member `__data`. */
char data[40];
};
In this scenario, we have two levels of indirection before reaching the
flexible array: `p->opt->opt->__data`. As a result, we can't simply
change the type of the conflicting object `opt` to `struct ip_options_hdr`
as we might in a simpler case, because `opt` in `struct ip_options_data`
isn't a flexible structure itself but a composite structure
`struct ip_options_rcu` containing one (flexible structure).
So, we have a couple of alternatives to fix this issue:
a) The complex solution: Create an additional "header type" with the
help of `struct_group_tagged()`, and introduce another
flexible-array member, as follows:
1) Create a new type `struct ip_options_rcu_hdr` using
`struct_group_tagged()` within `struct ip_options_rcu`.
2) Change the type of `opt` from `struct ip_options` to `struct
ip_options_hdr`, removing the flexible array member from the newly
created `struct ip_options_rcu_hdr`.
3) Add a flexible-array member directly inside `struct ip_options_rcu`,
so it still has a flexible-array member at the end --which was
previously "removed" with the change from `struct ip_options opt;`
to `struct ip_options_hdr opt;`:
| struct ip_options_rcu {
| - struct rcu_head rcu;
| - struct ip_options opt;
| + struct_group_tagged(ip_options_rcu_hdr, hdr,
| + struct rcu_head rcu;
| + struct ip_options_hdr opt;
| + );
| + unsigned char __data[];
| };
4) Now we can change the type of the conflicting object `opt` in
`struct ip_options_data` from `struct ip_options_rcu` to `struct
ip_options_rcu_hdr`, and with this silence the -Wfamnae warnings:
| struct ip_options_data {
| - struct ip_options_rcu opt;
| - char data[40];
| + struct ip_options_rcu_hdr opt;
| + char data[40];
| };
5) Code churn: This approach may (it really does) require more
adjustments and refactoring than usual due to multiple type
changes and the extra flexible-array member.
b) A simpler solution: Inline `struct ip_options_rcu` directly into
`struct ip_options_data` using an anonymous structure. See below:
1) Expand `struct ip_options_rcu` with an anonymous structure named
`opt`, preserving access to `struct ip_options_rcu` members via
`opt`.
2) Change the type of `opt` in the anonymous structure from `struct
ip_options` to `struct ip_options_hdr`, excluding the flexible
array and thus making the warnings go away:
| struct ip_options_data {
| - struct ip_options_rcu opt;
| + struct {
| + struct rcu_head rcu;
| + struct ip_options_hdr opt;
| + } opt;
| char data[40];
| };
3) Code churn: This solution requires minimal refactoring beyond
type casting instances of `opt` back to its original type `struct
ip_options_rcu`, and a couple of `container_of()`s.
So, I've decided to implement solution b) and with this, fix 2554 of the
following warnings:
include/net/inet_sock.h:64:33: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
Additionally, we want to ensure that when new members need to be added
to the flexible structure, they are always included within the newly
created tagged struct. For this, we use `static_assert()`. This ensures
that the memory layout for both the flexible structure and the new tagged
struct is the same after any changes.
Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.org>
---
include/net/inet_sock.h | 40 ++++++++++++++++++++++++----------------
net/ipv4/icmp.c | 14 ++++++++++----
net/ipv4/ip_output.c | 7 +++++--
net/ipv4/ping.c | 2 +-
net/ipv4/raw.c | 2 +-
net/ipv4/udp.c | 2 +-
6 files changed, 42 insertions(+), 25 deletions(-)
diff --git a/include/net/inet_sock.h b/include/net/inet_sock.h
index 56d8bc5593d3..ba4e0e2bbea0 100644
--- a/include/net/inet_sock.h
+++ b/include/net/inet_sock.h
@@ -37,23 +37,28 @@
* @ts_needaddr - Need to record addr of outgoing dev
*/
struct ip_options {
- __be32 faddr;
- __be32 nexthop;
- unsigned char optlen;
- unsigned char srr;
- unsigned char rr;
- unsigned char ts;
- unsigned char is_strictroute:1,
- srr_is_hit:1,
- is_changed:1,
- rr_needaddr:1,
- ts_needtime:1,
- ts_needaddr:1;
- unsigned char router_alert;
- unsigned char cipso;
- unsigned char __pad2;
+ /* New members MUST be added within the struct_group() macro below. */
+ struct_group_tagged(ip_options_hdr, __hdr,
+ __be32 faddr;
+ __be32 nexthop;
+ unsigned char optlen;
+ unsigned char srr;
+ unsigned char rr;
+ unsigned char ts;
+ unsigned char is_strictroute:1,
+ srr_is_hit:1,
+ is_changed:1,
+ rr_needaddr:1,
+ ts_needtime:1,
+ ts_needaddr:1;
+ unsigned char router_alert;
+ unsigned char cipso;
+ unsigned char __pad2;
+ );
unsigned char __data[];
};
+static_assert(offsetof(struct ip_options, __data) == sizeof(struct ip_options_hdr),
+ "struct member likely outside of struct_group_tagged()");
struct ip_options_rcu {
struct rcu_head rcu;
@@ -61,7 +66,10 @@ struct ip_options_rcu {
};
struct ip_options_data {
- struct ip_options_rcu opt;
+ struct {
+ struct rcu_head rcu;
+ struct ip_options_hdr opt;
+ } opt;
char data[40];
};
diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
index 33eec844a5a0..6a0a38c33cae 100644
--- a/net/ipv4/icmp.c
+++ b/net/ipv4/icmp.c
@@ -400,6 +400,9 @@ static void icmp_push_reply(struct sock *sk,
static void icmp_reply(struct icmp_bxm *icmp_param, struct sk_buff *skb)
{
+ struct ip_options *opt =
+ container_of(&icmp_param->replyopts.opt.opt,
+ struct ip_options, __hdr);
struct ipcm_cookie ipc;
struct rtable *rt = skb_rtable(skb);
struct net *net = dev_net(rt->dst.dev);
@@ -412,7 +415,7 @@ static void icmp_reply(struct icmp_bxm *icmp_param, struct sk_buff *skb)
int type = icmp_param->data.icmph.type;
int code = icmp_param->data.icmph.code;
- if (ip_options_echo(net, &icmp_param->replyopts.opt.opt, skb))
+ if (ip_options_echo(net, opt, skb))
return;
/* Needed by both icmpv4_global_allow and icmp_xmit_lock */
@@ -436,7 +439,7 @@ static void icmp_reply(struct icmp_bxm *icmp_param, struct sk_buff *skb)
saddr = fib_compute_spec_dst(skb);
if (icmp_param->replyopts.opt.opt.optlen) {
- ipc.opt = &icmp_param->replyopts.opt;
+ ipc.opt = (struct ip_options_rcu *)&icmp_param->replyopts.opt;
if (ipc.opt->opt.srr)
daddr = icmp_param->replyopts.opt.opt.faddr;
}
@@ -592,6 +595,7 @@ static struct rtable *icmp_route_lookup(struct net *net, struct flowi4 *fl4,
void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
const struct ip_options *opt)
{
+ struct ip_options *opt_aux;
struct iphdr *iph;
int room;
struct icmp_bxm icmp_param;
@@ -605,6 +609,8 @@ void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
struct net *net;
struct sock *sk;
+ opt_aux = container_of(&icmp_param.replyopts.opt.opt, struct ip_options, __hdr);
+
if (!rt)
goto out;
@@ -719,7 +725,7 @@ void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
iph->tos;
mark = IP4_REPLY_MARK(net, skb_in->mark);
- if (__ip_options_echo(net, &icmp_param.replyopts.opt.opt, skb_in, opt))
+ if (__ip_options_echo(net, opt_aux, skb_in, opt))
goto out_unlock;
@@ -736,7 +742,7 @@ void __icmp_send(struct sk_buff *skb_in, int type, int code, __be32 info,
inet_sk(sk)->tos = tos;
ipcm_init(&ipc);
ipc.addr = iph->saddr;
- ipc.opt = &icmp_param.replyopts.opt;
+ ipc.opt = (struct ip_options_rcu *)&icmp_param.replyopts.opt;
ipc.sockc.mark = mark;
rt = icmp_route_lookup(net, &fl4, skb_in, iph, saddr,
diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 0065b1996c94..17eeb3e09e95 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -1604,6 +1604,7 @@ void ip_send_unicast_reply(struct sock *sk, const struct sock *orig_sk,
unsigned int len, u64 transmit_time, u32 txhash)
{
struct ip_options_data replyopts;
+ struct ip_options *opt;
struct ipcm_cookie ipc;
struct flowi4 fl4;
struct rtable *rt = skb_rtable(skb);
@@ -1612,7 +1613,9 @@ void ip_send_unicast_reply(struct sock *sk, const struct sock *orig_sk,
int err;
int oif;
- if (__ip_options_echo(net, &replyopts.opt.opt, skb, sopt))
+ opt = container_of(&replyopts.opt.opt, struct ip_options, __hdr);
+
+ if (__ip_options_echo(net, opt, skb, sopt))
return;
ipcm_init(&ipc);
@@ -1620,7 +1623,7 @@ void ip_send_unicast_reply(struct sock *sk, const struct sock *orig_sk,
ipc.sockc.transmit_time = transmit_time;
if (replyopts.opt.opt.optlen) {
- ipc.opt = &replyopts.opt;
+ ipc.opt = (struct ip_options_rcu *)&replyopts.opt;
if (replyopts.opt.opt.srr)
daddr = replyopts.opt.opt.faddr;
diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index 619ddc087957..ef853dc3948f 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -753,7 +753,7 @@ static int ping_v4_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
if (inet_opt) {
memcpy(&opt_copy, inet_opt,
sizeof(*inet_opt) + inet_opt->opt.optlen);
- ipc.opt = &opt_copy.opt;
+ ipc.opt = (struct ip_options_rcu *)&opt_copy.opt;
}
rcu_read_unlock();
}
diff --git a/net/ipv4/raw.c b/net/ipv4/raw.c
index 0e9e01967ec9..cea8a559132f 100644
--- a/net/ipv4/raw.c
+++ b/net/ipv4/raw.c
@@ -563,7 +563,7 @@ static int raw_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
if (inet_opt) {
memcpy(&opt_copy, inet_opt,
sizeof(*inet_opt) + inet_opt->opt.optlen);
- ipc.opt = &opt_copy.opt;
+ ipc.opt = (struct ip_options_rcu *)&opt_copy.opt;
}
rcu_read_unlock();
}
diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
index 0e24916b39d4..6ff2329aa0fa 100644
--- a/net/ipv4/udp.c
+++ b/net/ipv4/udp.c
@@ -1159,7 +1159,7 @@ int udp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
if (inet_opt) {
memcpy(&opt_copy, inet_opt,
sizeof(*inet_opt) + inet_opt->opt.optlen);
- ipc.opt = &opt_copy.opt;
+ ipc.opt = (struct ip_options_rcu *)&opt_copy.opt;
}
rcu_read_unlock();
}
--
2.43.0
Powered by blists - more mailing lists