netdev - Re: [PATCH 4/5] net/packet: fix overflow in check for tp

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] [day] [month] [year] [list]

Message-ID: <CAF=yD-+1q+mP+vLDWsQan4ffnHSUqKn_aEjWJtad2Gd9eVXUDg@mail.gmail.com>
Date:   Tue, 28 Mar 2017 11:21:05 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     Andrey Konovalov <andreyknvl@...gle.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Willem de Bruijn <willemb@...gle.com>,
        Craig Gallek <cgallek@...gle.com>,
        Network Development <netdev@...r.kernel.org>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Kostya Serebryany <kcc@...gle.com>
Subject: Re: [PATCH 4/5] net/packet: fix overflow in check for tp_reserve

On Tue, Mar 28, 2017 at 11:11 AM, Andrey Konovalov
<andreyknvl@...gle.com> wrote:
> On Tue, Mar 28, 2017 at 5:00 PM, Willem de Bruijn
> <willemdebruijn.kernel@...il.com> wrote:
>> On Tue, Mar 28, 2017 at 10:00 AM, Andrey Konovalov
>> <andreyknvl@...gle.com> wrote:
>>> When calculating po->tp_hdrlen + po->tp_reserve the result can overflow.
>>>
>>> Fix by checking that tp_reserve <= INT_MAX on assign.
>>>
>>> This also takes cared of an overflow when calculating
>>>  macoff = TPACKET_ALIGN(po->tp_hdrlen) + 16 + po->tp_reserve
>>>  snaplen = skb->len
>>>  macoff + snaplen
>>> since macoff ~ INT_MAX and snaplen < SKB_MAX_ALLOC.
>>
>> This refers to the overflow of macoff + snaplen?
>>
>> Note that macoff is unsigned short, so will truncate any overflow from
>> tp_reserve.
>
> Yes, you're right.
> Should I make macoff unsigned int to fix this?

This is an unrelated issue. On first read, it seems quite harmless as
a process can
cause data to be placed at an offset that causes it to be overwritten
by the tpacket_hdr
later. Worth looking into more closely separately.

>>
>>> Signed-off-by: Andrey Konovalov <andreyknvl@...gle.com>
>>> ---
>>>  net/packet/af_packet.c | 7 ++++---
>>>  1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
>>> index c5c43fff8c01..28b49749d1af 100644
>>> --- a/net/packet/af_packet.c
>>> +++ b/net/packet/af_packet.c
>>> @@ -3665,6 +3665,8 @@ packet_setsockopt(struct socket *sock, int level, int optname, char __user *optv
>>>                         return -EBUSY;
>>>                 if (copy_from_user(&val, optval, sizeof(val)))
>>>                         return -EFAULT;
>>> +               if (val > INT_MAX)
>>> +                       return -EINVAL;
>>
>> This change on its own is sufficient to avoid the overflow. For net
>> and backports to stable, this minimal patch is preferable.
>
> I will put it into a separate patch then.

Thanks.

>
>>
>>>                 po->tp_reserve = val;
>>>                 return 0;
>>>         }
>>> @@ -4200,6 +4202,8 @@ static int packet_set_ring(struct sock *sk, union tpacket_req_u *req_u,
>>>                 if (unlikely((u64)req->tp_block_size * req->tp_block_nr >
>>>                                         UINT_MAX))
>>>                         goto out;
>>> +               if (unlikely(po->tp_reserve >= req->tp_frame_size))
>>> +                       goto out;
>>>
>>>                 if (unlikely(!PAGE_ALIGNED(req->tp_block_size)))
>>>                         goto out;
>>> @@ -4207,9 +4211,6 @@ static int packet_set_ring(struct sock *sk, union tpacket_req_u *req_u,
>>>                     req->tp_block_size <=
>>>                           BLK_PLUS_PRIV((u64)req_u->req3.tp_sizeof_priv))
>>>                         goto out;
>>> -               if (unlikely(req->tp_frame_size < po->tp_hdrlen +
>>> -                                       po->tp_reserve))
>>> -                       goto out;
>>
>> Is there a reason that the test is moved up? It is probably not
>> correct to remove tp_hdrlen from the test.
>
> Just to group together all checks of tp_frame_size and tp_block_size.

That makes sense, but indeed more for net-next. I would then send a single patch
that includes the other new block and frame tests.

> I'm not sure there's any difference between checking against
> po->tp_hdrlen + po->tp_reserve and just po->tp_reserve.
> I guess the correct check should be against
> TPACKET_ALIGN(po->tp_hdrlen) + 16 + po->tp_reserve.
>
> Should I use this value?

Yes, for net-next this seems like a good tightening of the test.