On 10/8/20 10:30 AM, Hangbin Liu wrote:

Hi Eric,

Thanks for the comments. I should add "RFC" in subject next time for the
uncertain fix patch.

On Wed, Oct 07, 2020 at 11:35:41AM +0200, Eric Dumazet wrote:

quoted

On 10/7/20 5:55 AM, Hangbin Liu wrote:

quoted

 		kfree_skb(skb);

@@ -282,6 +285,21 @@ static struct sk_buff *ip6_rcv_core(struct sk_buff *skb, struct net_device *dev,
 		}
 	}
 
+	/* RFC 8200, Section 4.5 Fragment Header:
+	 * If the first fragment does not include all headers through an
+	 * Upper-Layer header, then that fragment should be discarded and
+	 * an ICMP Parameter Problem, Code 3, message should be sent to
+	 * the source of the fragment, with the Pointer field set to zero.
+	 */
+	nexthdr = hdr->nexthdr;
+	offset = ipv6_skip_exthdr(skb, skb_transport_offset(skb), &nexthdr, &frag_off);
+	if (frag_off == htons(IP6_MF) && !pskb_may_pull(skb, offset + 1)) {
+		__IP6_INC_STATS(net, idev, IPSTATS_MIB_INHDRERRORS);
+		icmpv6_param_prob(skb, ICMPV6_HDR_INCOMP, 0);
+		rcu_read_unlock();
+		return NULL;
+	}
+
 	rcu_read_unlock();
 
 	/* Must drop socket now because of tproxy. */

Ouch, this is quite a buggy patch.

I doubt we want to add yet another ipv6_skip_exthdr() call in IPv6 fast path.

Surely the presence of NEXTHDR_FRAGMENT is already tested elsewhere ?

Would you like to help point where NEXTHDR_FRAGMENT was tested before IPv6
defragment?

I think we have to ask the question : Should routers enforce the rule, or
only end points ?

End points must handle NEXTHDR_FRAGMENT, in ipv6_frag_rcv()

quoted

Also, ipv6_skip_exthdr() does not pull anything in skb->head, it would be strange
to force a pull of hundreds of bytes just because you want to check if an extra byte is there,
if the packet could be forwarded as is, without additional memory allocations.

Testing skb->len should be more than enough at this stage.

Ah, yes, I shouldn't call pskb_may_pull here.

quoted

Also ipv6_skip_exthdr() can return an error.

it returns -1 as error, If we tested it by (offset + 1 > skb->len), does
that count as an error handler?

If there is an error, then you want to send the ICMP, right ?

The (offset + 1) expression will become 0, and surely the test will be false,
so you wont send the ICMP...

Thanks
Hangbin