netdev - Re: [PATCH net-next v4] ipv6: Fix signed integer overflow in __ip6_append

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 6 Jun 2022 10:03:27 +0800
From:   wangyufen <wangyufen@...wei.com>
To:     Paolo Abeni <pabeni@...hat.com>, Jakub Kicinski <kuba@...nel.org>
CC:     <davem@...emloft.net>, <yoshfuji@...ux-ipv6.org>,
        <dsahern@...nel.org>, <edumazet@...gle.com>, <ast@...nel.org>,
        <daniel@...earbox.net>, <andrii@...nel.org>, <kafai@...com>,
        <songliubraving@...com>, <yhs@...com>, <john.fastabend@...il.com>,
        <kpsingh@...nel.org>, <netdev@...r.kernel.org>,
        <bpf@...r.kernel.org>
Subject: Re: [PATCH net-next v4] ipv6: Fix signed integer overflow in
 __ip6_append_data


在 2022/6/3 16:58, Paolo Abeni 写道:
> On Thu, 2022-06-02 at 09:02 -0700, Jakub Kicinski wrote:
>> On Thu, 02 Jun 2022 12:38:10 +0200 Paolo Abeni wrote:
>>> I'm sorry for the multiple incremental feedback on this patch. It's
>>> somewhat tricky.
>>>
>>> AFAICS Jakub mentioned only udpv6_sendmsg(). In l2tp_ip6_sendmsg() we
>>> can have an overflow:
>>>
>>>          int transhdrlen = 4; /* zero session-id */
>>>          int ulen = len + transhdrlen;
>>>
>>> when len >= INT_MAX - 4. That will be harmless, but I guess it could
>>> still trigger a noisy UBSAN splat.
>> Good point, I wonder if that's a separate issue. Should we
>> follow what UDP does and subtract the transhdr from the max?
>> My gut feeling is that stricter checks are cleaner than just
>> bumping variable sizes.
>>
>> diff --git a/net/l2tp/l2tp_ip6.c b/net/l2tp/l2tp_ip6.c
>> index c6ff8bf9b55f..9dbd801ddb98 100644
>> --- a/net/l2tp/l2tp_ip6.c
>> +++ b/net/l2tp/l2tp_ip6.c
>> @@ -504,14 +504,15 @@ static int l2tp_ip6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>>          struct ipcm6_cookie ipc6;
>>          int addr_len = msg->msg_namelen;
>>          int transhdrlen = 4; /* zero session-id */
>> -       int ulen = len + transhdrlen;
>> +       int ulen;
>>          int err;
>>   
>>          /* Rough check on arithmetic overflow,
>>           * better check is made in ip6_append_data().
>>           */
>> -       if (len > INT_MAX)
>> +       if (len > INT_MAX - transhdrlen)
>>                  return -EMSGSIZE;
>> +       ulen = len + transhdrlen;
>>   
>>          /* Mirror BSD error message compatibility */
>>          if (msg->msg_flags & MSG_OOB)
>>
> LGTM. Imho this can even land in a separated patch (whatever is easier)

Thanks for all the feedback.
So, Jakub will send a new patch to fix the l2tp_ip6_sendmsg issue?

Thanks.