| Message ID | 20240510072932.2678952-1-edumazet@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1af7f88af269c4e06a4dc3bc920ff6cdf7471124 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] inet: fix inet_fill_ifaddr() flags truncation | expand |
On Fri, May 10, 2024 at 07:29:32AM +0000, Eric Dumazet wrote: > I missed that (struct ifaddrmsg)->ifa_flags was only 8bits, > while (struct in_ifaddr)->ifa_flags is 32bits. > > Use a temporary 32bit variable as I did in set_ifa_lifetime() > and check_lifetime(). > > Fixes: 3ddc2231c810 ("inet: annotate data-races around ifa->ifa_flags") > Reported-by: Yu Watanabe <watanabe.yu@gmail.com> > Dianosed-by: Yu Watanabe <watanabe.yu@gmail.com> > Closes: https://github.com/systemd/systemd/pull/32666#issuecomment-2103977928 Reviewed-by: Larysa Zaremba <larysa.zaremba@intel.com> > Signed-off-by: Eric Dumazet <edumazet@google.com> > --- > net/ipv4/devinet.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c > index 7a437f0d41905e6acfdc35743afba3a7abfd0dd5..7e45c34c8340a6d2cf96b4485cd4249fd4da7009 100644 > --- a/net/ipv4/devinet.c > +++ b/net/ipv4/devinet.c > @@ -1683,6 +1683,7 @@ static int inet_fill_ifaddr(struct sk_buff *skb, const struct in_ifaddr *ifa, > struct nlmsghdr *nlh; > unsigned long tstamp; > u32 preferred, valid; > + u32 flags; > > nlh = nlmsg_put(skb, args->portid, args->seq, args->event, sizeof(*ifm), > args->flags); > @@ -1692,7 +1693,13 @@ static int inet_fill_ifaddr(struct sk_buff *skb, const struct in_ifaddr *ifa, > ifm = nlmsg_data(nlh); > ifm->ifa_family = AF_INET; > ifm->ifa_prefixlen = ifa->ifa_prefixlen; > - ifm->ifa_flags = READ_ONCE(ifa->ifa_flags); > + > + flags = READ_ONCE(ifa->ifa_flags); > + /* Warning : ifm->ifa_flags is an __u8, it holds only 8 bits. > + * The 32bit value is given in IFA_FLAGS attribute. > + */ > + ifm->ifa_flags = (__u8)flags; > + > ifm->ifa_scope = ifa->ifa_scope; > ifm->ifa_index = ifa->ifa_dev->dev->ifindex; > > @@ -1701,7 +1708,7 @@ static int inet_fill_ifaddr(struct sk_buff *skb, const struct in_ifaddr *ifa, > goto nla_put_failure; > > tstamp = READ_ONCE(ifa->ifa_tstamp); > - if (!(ifm->ifa_flags & IFA_F_PERMANENT)) { > + if (!(flags & IFA_F_PERMANENT)) { > preferred = READ_ONCE(ifa->ifa_preferred_lft); > valid = READ_ONCE(ifa->ifa_valid_lft); > if (preferred != INFINITY_LIFE_TIME) { > @@ -1732,7 +1739,7 @@ static int inet_fill_ifaddr(struct sk_buff *skb, const struct in_ifaddr *ifa, > nla_put_string(skb, IFA_LABEL, ifa->ifa_label)) || > (ifa->ifa_proto && > nla_put_u8(skb, IFA_PROTO, ifa->ifa_proto)) || > - nla_put_u32(skb, IFA_FLAGS, ifm->ifa_flags) || > + nla_put_u32(skb, IFA_FLAGS, flags) || > (ifa->ifa_rt_priority && > nla_put_u32(skb, IFA_RT_PRIORITY, ifa->ifa_rt_priority)) || > put_cacheinfo(skb, READ_ONCE(ifa->ifa_cstamp), tstamp, > -- > 2.45.0.118.g7fe29c98d7-goog > >
On 5/10/24 1:29 AM, Eric Dumazet wrote: > I missed that (struct ifaddrmsg)->ifa_flags was only 8bits, > while (struct in_ifaddr)->ifa_flags is 32bits. > > Use a temporary 32bit variable as I did in set_ifa_lifetime() > and check_lifetime(). > > Fixes: 3ddc2231c810 ("inet: annotate data-races around ifa->ifa_flags") > Reported-by: Yu Watanabe <watanabe.yu@gmail.com> > Dianosed-by: Yu Watanabe <watanabe.yu@gmail.com> > Closes: https://github.com/systemd/systemd/pull/32666#issuecomment-2103977928 > Signed-off-by: Eric Dumazet <edumazet@google.com> > --- > net/ipv4/devinet.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > Reviewed-by: David Ahern <dsahern@kernel.org>
Hello: This patch was applied to netdev/net.git (main) by Jakub Kicinski <kuba@kernel.org>: On Fri, 10 May 2024 07:29:32 +0000 you wrote: > I missed that (struct ifaddrmsg)->ifa_flags was only 8bits, > while (struct in_ifaddr)->ifa_flags is 32bits. > > Use a temporary 32bit variable as I did in set_ifa_lifetime() > and check_lifetime(). > > Fixes: 3ddc2231c810 ("inet: annotate data-races around ifa->ifa_flags") > Reported-by: Yu Watanabe <watanabe.yu@gmail.com> > Dianosed-by: Yu Watanabe <watanabe.yu@gmail.com> > Closes: https://github.com/systemd/systemd/pull/32666#issuecomment-2103977928 > Signed-off-by: Eric Dumazet <edumazet@google.com> > > [...] Here is the summary with links: - [net] inet: fix inet_fill_ifaddr() flags truncation https://git.kernel.org/netdev/net/c/1af7f88af269 You are awesome, thank you!
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c index 7a437f0d41905e6acfdc35743afba3a7abfd0dd5..7e45c34c8340a6d2cf96b4485cd4249fd4da7009 100644 --- a/net/ipv4/devinet.c +++ b/net/ipv4/devinet.c @@ -1683,6 +1683,7 @@ static int inet_fill_ifaddr(struct sk_buff *skb, const struct in_ifaddr *ifa, struct nlmsghdr *nlh; unsigned long tstamp; u32 preferred, valid; + u32 flags; nlh = nlmsg_put(skb, args->portid, args->seq, args->event, sizeof(*ifm), args->flags); @@ -1692,7 +1693,13 @@ static int inet_fill_ifaddr(struct sk_buff *skb, const struct in_ifaddr *ifa, ifm = nlmsg_data(nlh); ifm->ifa_family = AF_INET; ifm->ifa_prefixlen = ifa->ifa_prefixlen; - ifm->ifa_flags = READ_ONCE(ifa->ifa_flags); + + flags = READ_ONCE(ifa->ifa_flags); + /* Warning : ifm->ifa_flags is an __u8, it holds only 8 bits. + * The 32bit value is given in IFA_FLAGS attribute. + */ + ifm->ifa_flags = (__u8)flags; + ifm->ifa_scope = ifa->ifa_scope; ifm->ifa_index = ifa->ifa_dev->dev->ifindex; @@ -1701,7 +1708,7 @@ static int inet_fill_ifaddr(struct sk_buff *skb, const struct in_ifaddr *ifa, goto nla_put_failure; tstamp = READ_ONCE(ifa->ifa_tstamp); - if (!(ifm->ifa_flags & IFA_F_PERMANENT)) { + if (!(flags & IFA_F_PERMANENT)) { preferred = READ_ONCE(ifa->ifa_preferred_lft); valid = READ_ONCE(ifa->ifa_valid_lft); if (preferred != INFINITY_LIFE_TIME) { @@ -1732,7 +1739,7 @@ static int inet_fill_ifaddr(struct sk_buff *skb, const struct in_ifaddr *ifa, nla_put_string(skb, IFA_LABEL, ifa->ifa_label)) || (ifa->ifa_proto && nla_put_u8(skb, IFA_PROTO, ifa->ifa_proto)) || - nla_put_u32(skb, IFA_FLAGS, ifm->ifa_flags) || + nla_put_u32(skb, IFA_FLAGS, flags) || (ifa->ifa_rt_priority && nla_put_u32(skb, IFA_RT_PRIORITY, ifa->ifa_rt_priority)) || put_cacheinfo(skb, READ_ONCE(ifa->ifa_cstamp), tstamp,
I missed that (struct ifaddrmsg)->ifa_flags was only 8bits, while (struct in_ifaddr)->ifa_flags is 32bits. Use a temporary 32bit variable as I did in set_ifa_lifetime() and check_lifetime(). Fixes: 3ddc2231c810 ("inet: annotate data-races around ifa->ifa_flags") Reported-by: Yu Watanabe <watanabe.yu@gmail.com> Dianosed-by: Yu Watanabe <watanabe.yu@gmail.com> Closes: https://github.com/systemd/systemd/pull/32666#issuecomment-2103977928 Signed-off-by: Eric Dumazet <edumazet@google.com> --- net/ipv4/devinet.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-)