[PATCH net] af_packet: fix integer overflow in prb_calc_retire_blk_tmo()

[PATCH net] af_packet: fix integer overflow in prb_calc_retire_blk_tmo()

[Dairui Zhang]

prb_calc_retire_blk_tmo() computes in 32-bit int arithmetic:

        mbits = (blk_size_in_bytes * 8) / (1024 * 1024);

If I'm reading the validation right, tp_block_size is user
controlled and packet_set_ring() only rejects values that are <= 0
as int or not page aligned, so a 256MiB block goes right through
(and alloc_one_pg_vec_page() even has a vzalloc fallback for it).
0x10000000 * 8 wraps to INT_MIN, and on a NIC reporting 1 Gbps
(div == 1) the function ends up returning -2047.

The condition is actually (8 * size) mod 2^32 >= 2^31 && div == 1,
so the trigger set is [256,512), [768,1024), [1280,1536) and
[1792,2048) MiB. Other sizes wrap to non-negative values and faster
links divide the unsigned value back below 2^31, which is why this
doesn't blow up for everyone.

What makes it fatal is what happens next in init_prb_bdqc():

        p1->interval_ktime = ms_to_ktime(prb_calc_retire_blk_tmo(...));
        hrtimer_start(&p1->retire_blk_timer, p1->interval_ktime,
                      HRTIMER_MODE_REL_SOFT);

A negative relative timeout expires immediately. The callback
unconditionally returns HRTIMER_RESTART, and hrtimer_forward() turns
the negative interval into hrtimer_resolution:

        if (interval < hrtimer_resolution)
                interval = hrtimer_resolution;

So the SOFT timer re-fires at the maximum rate forever, holding
sk_receive_queue.lock each pass. One CPU spins in softirq until the
socket is closed. Repeat with more rings and the machine is gone.

This is mostly a new problem: the overflow is ancient, but with
msecs_to_jiffies() a negative timeout just got clamped to a huge
unsigned value, i.e. the retire timer effectively never fired.
f7460d2989fa ("net: af_packet: Use hrtimer to do the retire
operation", v6.18) turned it into "fire immediately and spin".
Still present in 7.2.6 and in current mainline as of this
writing.

(Unrelated to CVE-2019-20812 - that one was the ethtool failure path
returning 0, which now returns DEFAULT_PRB_RETIRE_TOV.)

Reproducer, needs CAP_NET_RAW (a --network host container has it by
default) and a 1 Gbps NIC (QEMU e1000 works):

        int fd = socket(AF_PACKET, SOCK_RAW, htons(ETH_P_ALL));
        bind(fd, ...);
        int v = TPACKET_V3;
        setsockopt(fd, SOL_PACKET, PACKET_VERSION, &v, sizeof(v));
        struct tpacket_req3 req = {
                .tp_block_size = 0x10000000,
                .tp_block_nr = 1,
                .tp_frame_size = 2048,
                .tp_frame_nr = 0x10000000 / 2048,
                .tp_retire_blk_tov = 0,
        };
        setsockopt(fd, SOL_PACKET, PACKET_RX_RING, &req, sizeof(req));

Compute in 64 bits instead. The operands are already bounded by the
existing validation, so nothing else changes. If you'd prefer a
different fix, just say so and I'll respin.

Fixes: f7460d2989fa ("net: af_packet: Use hrtimer to do the retire operation")
Cc: stable@vger.kernel.org
Signed-off-by: Dairui Zhang <zhangdairui@gmail.com>
---
First patch to netdev, and compile-tested only - I don't have a
1 Gbps setup to trigger it live. If I've misread the code, got the
Fixes: tag wrong, or picked the wrong fix, please say so and I'll
respin.

 net/packet/af_packet.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index b22cda3..24cf2d2 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -617,7 +617,7 @@ static int prb_calc_retire_blk_tmo(struct packet_sock *po,
 		return DEFAULT_PRB_RETIRE_TOV;
 
 	div = ecmd.base.speed / 1000;
-	mbits = (blk_size_in_bytes * 8) / (1024 * 1024);
+	mbits = (u64)blk_size_in_bytes * 8 / (1024 * 1024);
 
 	if (div)
 		mbits /= div;

Re: [PATCH net] af_packet: fix integer overflow in prb_calc_retire_blk_tmo()

[Willem de Bruijn]

Dairui Zhang wrote:
> prb_calc_retire_blk_tmo() computes in 32-bit int arithmetic:
> 
>         mbits = (blk_size_in_bytes * 8) / (1024 * 1024);
> 
> If I'm reading the validation right, tp_block_size is user
> controlled and packet_set_ring() only rejects values that are <= 0
> as int or not page aligned, so a 256MiB block goes right through
> (and alloc_one_pg_vec_page() even has a vzalloc fallback for it).
> 0x10000000 * 8 wraps to INT_MIN, and on a NIC reporting 1 Gbps
> (div == 1) the function ends up returning -2047.
> 
> The condition is actually (8 * size) mod 2^32 >= 2^31 && div == 1,
> so the trigger set is [256,512), [768,1024), [1280,1536) and
> [1792,2048) MiB. Other sizes wrap to non-negative values and faster
> links divide the unsigned value back below 2^31, which is why this
> doesn't blow up for everyone.
> 
> What makes it fatal is what happens next in init_prb_bdqc():
> 
>         p1->interval_ktime = ms_to_ktime(prb_calc_retire_blk_tmo(...));
>         hrtimer_start(&p1->retire_blk_timer, p1->interval_ktime,
>                       HRTIMER_MODE_REL_SOFT);
> 
> A negative relative timeout expires immediately. The callback
> unconditionally returns HRTIMER_RESTART, and hrtimer_forward() turns
> the negative interval into hrtimer_resolution:
> 
>         if (interval < hrtimer_resolution)
>                 interval = hrtimer_resolution;
> 
> So the SOFT timer re-fires at the maximum rate forever, holding
> sk_receive_queue.lock each pass. One CPU spins in softirq until the
> socket is closed. Repeat with more rings and the machine is gone.
> 
> This is mostly a new problem: the overflow is ancient, but with
> msecs_to_jiffies() a negative timeout just got clamped to a huge
> unsigned value, i.e. the retire timer effectively never fired.
> f7460d2989fa ("net: af_packet: Use hrtimer to do the retire
> operation", v6.18) turned it into "fire immediately and spin".
> Still present in 7.2.6 and in current mainline as of this
> writing.
> 
> (Unrelated to CVE-2019-20812 - that one was the ethtool failure path
> returning 0, which now returns DEFAULT_PRB_RETIRE_TOV.)
> 
> Reproducer, needs CAP_NET_RAW (a --network host container has it by
> default) and a 1 Gbps NIC (QEMU e1000 works):
> 
>         int fd = socket(AF_PACKET, SOCK_RAW, htons(ETH_P_ALL));
>         bind(fd, ...);
>         int v = TPACKET_V3;
>         setsockopt(fd, SOL_PACKET, PACKET_VERSION, &v, sizeof(v));
>         struct tpacket_req3 req = {
>                 .tp_block_size = 0x10000000,
>                 .tp_block_nr = 1,
>                 .tp_frame_size = 2048,
>                 .tp_frame_nr = 0x10000000 / 2048,
>                 .tp_retire_blk_tov = 0,
>         };
>         setsockopt(fd, SOL_PACKET, PACKET_RX_RING, &req, sizeof(req));
> 
> Compute in 64 bits instead. The operands are already bounded by the
> existing validation, so nothing else changes. If you'd prefer a
> different fix, just say so and I'll respin.
> 
> Fixes: f7460d2989fa ("net: af_packet: Use hrtimer to do the retire operation")
> Cc: stable@vger.kernel.org
> Signed-off-by: Dairui Zhang <zhangdairui@gmail.com>
> ---
> First patch to netdev, and compile-tested only - I don't have a
> 1 Gbps setup to trigger it live. If I've misread the code, got the
> Fixes: tag wrong, or picked the wrong fix, please say so and I'll
> respin.

Code Looks good to me. Thanks.

As Fixes tag do prefer original commit f6fb8f100b80 ("af-packet:
TPACKET_V3 flexible buffer implementation.")

That is where the bug is introduced, and the effect of the bug prior
to the currenty blamed commit is not as clear-cut as described in the
commit message.

The return value of prb_calc_retire_blk_tmo() is stored into an
unsigned short retire_blk_tov then, so can be truncated. A 0-jiffy
delay loop can also be programmed. Though its effect is not nearly as
detrimental as from the blamed commit on.

Re: [PATCH net] af_packet: fix integer overflow in prb_calc_retire_blk_tmo()

[Dairui Zhang]

Willem de Bruijn wrote:
> Code Looks good to me. Thanks.
>
> As Fixes tag do prefer original commit f6fb8f100b80 ("af-packet:
> TPACKET_V3 flexible buffer implementation.")

Thanks for the quick review, and for the history correction - the
unsigned short truncation detail is exactly the kind of thing I
couldn't have known without your pointer. I'll respin as v2 with
Fixes: pointing at f6fb8f100b80 and the pre-hrtimer effect
described as you outlined, after the 24h wait.

Thanks,
Dairui Zhang