svn commit: r358167 - head/sys/netinet6

[Bjoern A. Zeeb]

On 25 Feb 2020, at 12:44, Kristof Provost wrote:

>> This change introduces a slight regression when a host replies to 
>> IPv6 ping fragmented packets. The problem is the "unfragpartlen" must 
>> also be set in the else case of "if (opt)", else the payload offset 
>> computation for IPv6 fragments goes wrong by the size of the IPv6 
>> header!
>>
> Confirmed, because the pf fragmentation:v6 test also fails on this.


>> Patch goes like this:
>>
>>> diff --git a/sys/netinet6/ip6_output.c b/sys/netinet6/ip6_output.c
>>> index 06c57bcec48..a6c8d148833 100644
>>> --- a/sys/netinet6/ip6_output.c
>>> +++ b/sys/netinet6/ip6_output.c
>>> @@ -459,7 +459,6 @@ ip6_output(struct mbuf *m0, struct ip6_pktopts 
>>> *opt,
>>>          */
>>>         bzero(&exthdrs, sizeof(exthdrs));
>>>         optlen = 0;
>>> -       unfragpartlen = 0;
>>>         if (opt) {
>>>                 /* Hop-by-Hop options header. */
>>>                 MAKE_EXTHDR(opt->ip6po_hbh, &exthdrs.ip6e_hbh, 
>>> optlen);
>>> @@ -497,8 +496,6 @@ ip6_output(struct mbuf *m0, struct ip6_pktopts 
>>> *opt,
>>>                 /* Routing header. */
>>>                 MAKE_EXTHDR(opt->ip6po_rthdr, &exthdrs.ip6e_rthdr, 
>>> optlen);
>>> -               unfragpartlen = optlen + sizeof(struct ip6_hdr);
>>> -
>>>                 /*
>>>                  * NOTE: we don't add AH/ESP length here (done in
>>>                  * ip6_ipsec_output()).
>>> @@ -508,6 +505,8 @@ ip6_output(struct mbuf *m0, struct ip6_pktopts 
>>> *opt,
>>>                 MAKE_EXTHDR(opt->ip6po_dest2, &exthdrs.ip6e_dest2, 
>>> optlen);
>>>         }
>>> +       unfragpartlen = optlen + sizeof(struct ip6_hdr);
>>> +
>>>         /*
>>>          * If there is at least one extension header,
>>>          * separate IP6 header from the payload.
>>
> And with this patch the test passes again.

And fails for other packets.  The patch is wrong.

Offset gets changed after we set unfragpartlen inside the block so 
moving it outside the block unfragpartlen may point to the wrong thing.

Your tests are too simple to detect this problem.

Try this one please:

Index: ip6_output.c
===================================================================
--- ip6_output.c        (revision 358297)
+++ ip6_output.c        (working copy)
@@ -497,7 +497,7 @@ ip6_output(struct mbuf *m0, struct ip6_pktopts *op
          */
         bzero(&exthdrs, sizeof(exthdrs));
         optlen = 0;
-       unfragpartlen = 0;
+       unfragpartlen = sizeof(struct ip6_hdr);
         if (opt) {
                 /* Hop-by-Hop options header. */
                 MAKE_EXTHDR(opt->ip6po_hbh, &exthdrs.ip6e_hbh, optlen);
@@ -535,7 +535,7 @@ ip6_output(struct mbuf *m0, struct ip6_pktopts *op
                 /* Routing header. */
                 MAKE_EXTHDR(opt->ip6po_rthdr, &exthdrs.ip6e_rthdr, 
optlen);

-               unfragpartlen = optlen + sizeof(struct ip6_hdr);
+               unfragpartlen += optlen;

                 /*
                  * NOTE: we don't add AH/ESP length here (done in


/bz