On Tue, Mar 28, 2017 at 11:11 AM, Andrey Konovalov <andreyk...@google.com> wrote: > On Tue, Mar 28, 2017 at 5:00 PM, Willem de Bruijn > <willemdebruijn.ker...@gmail.com> wrote: >> On Tue, Mar 28, 2017 at 10:00 AM, Andrey Konovalov >> <andreyk...@google.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 <andreyk...@google.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.
