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

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

Message-ID: <CAEfhGizS=cwEka=ouphw2S6yL-n4OzZ8msX=FOBO_UjnSAB+cw@mail.gmail.com>
Date:   Tue, 28 Mar 2017 11:54:58 -0400
From:   Craig Gallek <kraigatgoog@...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>,
        netdev <netdev@...r.kernel.org>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Kostya Serebryany <kcc@...gle.com>
Subject: Re: [PATCH 3/5] net/packet: fix overflow in check for tp_frame_nr

On Tue, Mar 28, 2017 at 10:00 AM, Andrey Konovalov
<andreyknvl@...gle.com> wrote:
> When calculating rb->frames_per_block * req->tp_block_nr the result
> can overflow.
>
> Add a check that tp_block_size * tp_block_nr <= UINT_MAX.
>
> Since frames_per_block <= tp_block_size, the expression would
> never overflow.
>
> Signed-off-by: Andrey Konovalov <andreyknvl@...gle.com>
> ---
>  net/packet/af_packet.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 506348abdf2f..c5c43fff8c01 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -4197,6 +4197,9 @@ static int packet_set_ring(struct sock *sk, union tpacket_req_u *req_u,
>                         goto out;
>                 if (unlikely(req->tp_frame_size == 0))
>                         goto out;
> +               if (unlikely((u64)req->tp_block_size * req->tp_block_nr >
> +                                       UINT_MAX))
> +                       goto out;
So this may be pedantic, but really the only guarantee that you have
for the 'unsigned int' type of these fields is that they are _at
least_ 16 bits.  There is no guarantee on the upper bound size, so
casting to a u64 will be problematic on a compiler that happens to use
64 bits for 'unsigned int'.  I'm not aware of any that use greater
than 32 bits right now and using one that does may very well break
other things in the kernel, but here we are...  Perhaps a alternative
fix would be to do the multiplication into an 'unsigned int' type and
ensure that the result is larger than each of the original two values?

The real issue is that explicit size types should have been used in
this userspace structure.