Messages in this thread Patch in this message | | | Date | Thu, 10 Mar 2022 13:14:38 -0800 | Subject | Re: [PATCH] net: ipv6: fix invalid alloclen in __ip6_append_data | From | Tadeusz Struk <> |
| |
On 3/10/22 09:32, Willem de Bruijn wrote: > On Thu, Mar 10, 2022 at 11:06 AM Tadeusz Struk <tadeusz.struk@linaro.org> wrote: >> >> On 3/10/22 06:39, Willem de Bruijn wrote: >>> On Wed, Mar 9, 2022 at 4:37 PM Tadeusz Struk <tadeusz.struk@linaro.org> wrote: >>>> >>>> On 3/8/22 21:01, David Ahern wrote: >>>>> On 3/8/22 12:46 PM, Tadeusz Struk wrote: >>>>>> That fails in the same way: >>>>>> >>>>>> skbuff: skb_over_panic: text:ffffffff83e7b48b len:65575 put:65575 >>>>>> head:ffff888101f8a000 data:ffff888101f8a088 tail:0x100af end:0x6c0 >>>>>> dev:<NULL> >>>>>> ------------[ cut here ]------------ >>>>>> kernel BUG at net/core/skbuff.c:113! >>>>>> invalid opcode: 0000 [#1] PREEMPT SMP KASAN >>>>>> CPU: 0 PID: 1852 Comm: repro Not tainted >>>>>> 5.17.0-rc7-00020-gea4424be1688-dirty #19 >>>>>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1.fc35 >>>>>> RIP: 0010:skb_panic+0x173/0x175 >>>>>> >>>>>> I'm not sure how it supposed to help since it doesn't change the >>>>>> alloclen at all. >>>>> >>>>> alloclen is a function of fraglen and fraglen is a function of datalen. >>>> >>>> Ok, but in this case it doesn't affect the alloclen and it still fails. >>> >>> This is some kind of non-standard packet that is being constructed. Do >>> we understand how it is different? >>> >>> The .syz reproducer is generally a bit more readable than the .c >>> equivalent. Though not as much as an strace of the binary, if you >>> can share that. >>> >>> r0 = socket$inet6_icmp_raw(0xa, 0x3, 0x3a) >>> connect$inet6(r0, &(0x7f0000000040)={0xa, 0x0, 0x0, @dev, 0x6}, 0x1c) >>> setsockopt$inet6_IPV6_HOPOPTS(r0, 0x29, 0x36, >>> &(0x7f0000000100)=ANY=[@ANYBLOB="52b3"], 0x5a0) >>> sendmmsg$inet(r0, &(0x7f00000002c0)=[{{0x0, 0x0, >>> &(0x7f0000000000)=[{&(0x7f00000000c0)="1d2d", 0xfa5f}], 0x1}}], 0x1, >>> 0xfe80) >> >> Here it is: >> https://termbin.com/krtr >> It won't be of much help, I'm afraid, as the offending sendmmsg() >> call isn't fully printed. > > Thanks. It does offer some hints on the other two syscalls: > > [pid 644] connect(3, {sa_family=AF_INET6, sin6_port=htons(0), > sin6_flowinfo=htonl(0), inet_pton(AF_INET6, "fe80::", &sin6_addr), > sin6_scope_id=if_nametoindex("tunl0")}, 28) = 0 > [pid 644] setsockopt(3, SOL_IPV6, IPV6_HOPOPTS, > "R\263\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., > 1440) = 0 > > IPV6_HOPOPTS is ns_capable CAP_NET_RAW. > > So this adds 1440 bytes to opt_nflen, which is included in > fragheaderlen, causing that to be exactly mtu. This means that the > payload can never be sent, as each fragment header eats up the entire > mtu? This is without any transport headers that would only be part of > the first fragment (which go into opt_flen). > > If you can maybe catch the error before the skb_put and just return > EINVAL, we might see whether sendmmsg is relevant or a simple send > would be equivalent. (not super important, that appears unrelated.)
Do you mean something like this: diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c index 622345af323e..6d45112322a0 100644 @@ -1656,6 +1649,16 @@ static int __ip6_append_data(struct sock *sk, skb->protocol = htons(ETH_P_IPV6); skb->ip_summed = csummode; skb->csum = 0; + + /* + * Check if there is still room for the payload + */ + if (fragheaderlen >= mtu) { + err = -EMSGSIZE; + kfree_skb(skb); + goto error; + } + /* reserve for fragmentation and ipsec header */ skb_reserve(skb, hh_len + sizeof(struct frag_hdr) + dst_exthdrlen);
That works as well. -- Thanks, Tadeusz
| |