Re: [PATCH 4/5] net/packet: fix overflow in check for tp_reserve

On Tue, Mar 28, 2017 at 11:11 AM, Andrey Konovalov
[off-list ref] wrote:

On Tue, Mar 28, 2017 at 5:00 PM, Willem de Bruijn
[off-list ref] wrote:

quoted

On Tue, Mar 28, 2017 at 10:00 AM, Andrey Konovalov
[off-list ref] wrote:

quoted

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.

quoted

quoted

Signed-off-by: Andrey Konovalov <redacted>
---
 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.

quoted

quoted

                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.