On Sun, Jul 05, 2020 at 11:18:36PM +0300, Nikolay Aleksandrov wrote: > > > By the way, I can't verify at the moment, but I think we can drop that whole > > > hunk altogether since skb_header_pointer() is used and it will simply return > > > an error if there isn't enough data for nsrcs. > > > > > > > Hm, while unlikely, the IPv6 packet / header payload length might be > > shorter than the frame / skb size. > > > > And even though it wouldn't crash reading over the IPv6 packet > > length, especially as skb_header_pointer() is used, I think we should > > still avoid reading over the size indicated by the IPv6 header payload > > length field, to stay protocol compliant. > > > > Does that make sense? > > > > Sure, I just thought the ipv6_mc_may_pull() call after that includes those 2 bytes as well, so > we're covered. That is, it seems to be doing the same check with the full grec size included. > Ah, okay, that's what you mean! You're right, technically the ipv6_mc_may_pull() later would cover it, too. And it should work, even if nsrcs were outside of the IPv6 packet and had a bogus value. (I think.) My brain linearly parsing the parser code would probably get a bit confused initially, as it'd look like a bit like a bug. But the current check for nsrcs might look confusing, too (q.e.d.). So as you prefer, I'd be okay with both leaving that check for consistency or removing it for brevity.
