Messages in this thread Patch in this message | | | From | Joy Gu <> | Subject | [PATCH] net: bridge: mcast: read ngrec once in igmp3/mld2 report | Date | Mon, 19 Dec 2022 18:48:07 -0800 |
| |
In br_ip4_multicast_igmp3_report() and br_ip6_multicast_mld2_report(), "ih" or "mld2r" is a pointer into the skb header. It's dereferenced to get "num", which is used in the for-loop condition that follows.
Compilers are free to not spend a register on "num" and dereference that pointer every time "num" would be used, i.e. every loop iteration. Which would be a bug if pskb_may_pull() (called by ip_mc_may_pull() or ipv6_mc_may_pull() in the loop body) were to change pointers pointing into the skb header, e.g. by freeing "skb->head".
We can avoid this by using READ_ONCE().
Suggested-by: Joern Engel <joern@purestorage.com> Signed-off-by: Joy Gu <jgu@purestorage.com> --- net/bridge/br_multicast.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c index 48170bd3785e..2ac4b099e00d 100644 --- a/net/bridge/br_multicast.c +++ b/net/bridge/br_multicast.c @@ -2624,11 +2624,11 @@ static int br_ip4_multicast_igmp3_report(struct net_bridge_mcast *brmctx, bool changed = false; int err = 0; u16 nsrcs; ih = igmpv3_report_hdr(skb); - num = ntohs(ih->ngrec); + num = ntohs(READ_ONCE(ih->ngrec)); len = skb_transport_offset(skb) + sizeof(*ih); for (i = 0; i < num; i++) { len += sizeof(*grec); if (!ip_mc_may_pull(skb, len)) @@ -2750,11 +2750,11 @@ static int br_ip6_multicast_mld2_report(struct net_bridge_mcast *brmctx, if (!ipv6_mc_may_pull(skb, sizeof(*mld2r))) return -EINVAL; mld2r = (struct mld2_report *)icmp6_hdr(skb); - num = ntohs(mld2r->mld2r_ngrec); + num = ntohs(READ_ONCE(mld2r->mld2r_ngrec)); len = skb_transport_offset(skb) + sizeof(*mld2r); for (i = 0; i < num; i++) { __be16 *_nsrcs, __nsrcs; u16 nsrcs; -- 2.39.0
| |