[PATCH] net: bridge: mcast: read ngrec once in igmp3/mld2 report

[PATCH] net: bridge: mcast: read ngrec once in igmp3/mld2 report

[Joy Gu]

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

Re: [PATCH] net: bridge: mcast: read ngrec once in igmp3/mld2 report

[Nikolay Aleksandrov]

On 20/12/2022 04:48, Joy Gu wrote:
> 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(-)
> 

I doubt any compiler would do that (partly due to the ntohs()). If you have hit a bug or
seen this with some compiler please provide more details, disassembly of the resulting
code would be best.

Thanks.

Re: [PATCH] net: bridge: mcast: read ngrec once in igmp3/mld2 report

[Paolo Abeni]

On Tue, 2022-12-20 at 12:13 +0200, Nikolay Aleksandrov wrote:
> On 20/12/2022 04:48, Joy Gu wrote:
> > 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(-)
> > 
> 
> I doubt any compiler would do that (partly due to the ntohs()). 

I would say that any compiler behaving as described above is buggy, as
'skb' is modified inside the loop, and the header pointer is fetched
from the skb, it can't be considered invariant.

> If you have hit a bug or
> seen this with some compiler please provide more details, disassembly of the resulting
> code would be best.

Exactly. A more detailed description of the issue you see is necessary.
And very likely the proposed solution is not the correct one.

Cheers,

Paolo