netdev - Re: [PATCH net-next 3/3] udp: only use paged allocation with scatter-gather

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:   Tue, 15 May 2018 19:57:28 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     Network Development <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>,
        Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net-next 3/3] udp: only use paged allocation with scatter-gather

On Tue, May 15, 2018 at 4:04 PM, Willem de Bruijn
<willemdebruijn.kernel@...il.com> wrote:
> On Tue, May 15, 2018 at 10:14 AM, Willem de Bruijn
> <willemdebruijn.kernel@...il.com> wrote:
>> On Mon, May 14, 2018 at 7:45 PM, Eric Dumazet <eric.dumazet@...il.com> wrote:
>>>
>>>
>>> On 05/14/2018 04:30 PM, Willem de Bruijn wrote:
>>>
>>>> I don't quite follow. The reported crash happens in the protocol layer,
>>>> because of this check. With pagedlen we have not allocated
>>>> sufficient space for the skb_put.
>>>>
>>>>                 if (!(rt->dst.dev->features&NETIF_F_SG)) {
>>>>                         unsigned int off;
>>>>
>>>>                         off = skb->len;
>>>>                         if (getfrag(from, skb_put(skb, copy),
>>>>                                         offset, copy, off, skb) < 0) {
>>>>                                 __skb_trim(skb, off);
>>>>                                 err = -EFAULT;
>>>>                                 goto error;
>>>>                         }
>>>>                 } else {
>>>>                         int i = skb_shinfo(skb)->nr_frags;
>>>>
>>>> Are you referring to a separate potential issue in the gso layer?
>>>> If a bonding device advertises SG, but a slave does not, then
>>>> skb_segment on the slave should build linear segs? I have not
>>>> tested that.
>>>
>>> Given that the device attribute could change under us, we need to not
>>> crash, even if initially we thought NETIF_F_SG was available.
>>>
>>> Unless you want to hold RTNL in UDP xmit :)
>>>
>>> Ideally, GSO should be always on, as we did for TCP.
>>>
>>> Otherwise, I can guarantee syzkaller will hit again.
>>
>> Ah, right. Thanks, Eric!
>>
>> I'll read that feature bit only once.
>
> This issue is actually deeper and not specific to gso.
> With corking it is trivial to turn off sg in between calls.
>
> I'll need to send a separate fix for that.

This would do it. The extra branch is unfortunate, but I see no easy
way around it for the corking case.

It will obviously not build a linear skb, but validate_xmit_skb will clean
that up for such edge cases.

diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c
index 66340ab750e6..e7daec7c7421 100644
--- a/net/ipv4/ip_output.c
+++ b/net/ipv4/ip_output.c
@@ -1040,7 +1040,8 @@ static int __ip_append_data(struct sock *sk,
                if (copy > length)
                        copy = length;

-               if (!(rt->dst.dev->features&NETIF_F_SG)) {
+               if (!(rt->dst.dev->features&NETIF_F_SG) &&
+                   skb_tailroom(skb) >= copy) {
                        unsigned int off;