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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message says the negative timeout needs `div == 1`,
  i.e. a 1 Gbps link.
- [Low] The commit message says the reproducer "needs CAP_NET_RAW (a
  --network host container has it by default) and a 1 Gbps NIC (QEMU…

--

af_packet: fix integer overflow in prb_calc_retire_blk_tmo()

This patch makes prb_calc_retire_blk_tmo() compute mbits in 64-bit
arithmetic. The old 32-bit int product could wrap negative for large
TPACKET_V3 block sizes. The negative result then armed the retire hrtimer
with a negative relative timeout, and the timer re-fired forever.

> 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.

[Severity: Medium]
Does the negative return really need div == 1?

In the pre-patch prb_calc_retire_blk_tmo(), a link reporting 2000-2999
Mbps (for example SPEED_2500, so div == 2) can also return a negative
value. The final unsigned increment is what pushes it over:

    div = ecmd.base.speed / 1000;
    mbits = (blk_size_in_bytes * 8) / (1024 * 1024);

    if (div)
        mbits /= div;

    if (div)
        return mbits + 1;

Take tp_block_size = 0x1ffe0000. It is positive as an int and page
aligned, so packet_set_ring() accepts it:

    blk_size_in_bytes * 8    = 0xfff00000 (-1048576 as int)
    / (1024 * 1024)          = -1, so mbits = UINT_MAX
    mbits /= 2               = 0x7fffffff
    return mbits + 1         = 0x80000000 (INT_MIN as int)

ms_to_ktime() takes a u64, so INT_MIN is sign-extended and multiplied by
NSEC_PER_MSEC, which gives a negative ktime. init_prb_bdqc() passes that
to hrtimer_start(). prb_retire_rx_blk_timer_expired() keeps returning
HRTIMER_RESTART, so the timer spins just as it does when div == 1.

More generally, with div == 2 the old int quotient only has to be -1 or
-2. That covers sizes about 128-384 KiB below each 512 MiB multiple
(512, 1024, 1536 and 2048 MiB).

The u64 arithmetic in this patch fixes these sizes too. Could the
description of the trigger condition be corrected, so that stable and
security triage also covers 2.5 Gbps links?

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

[Severity: Low]
Is the set of users who can reach this wider than described here?

packet_create() checks the capability against the user namespace that
owns the netns:

    if (!ns_capable(net->user_ns, CAP_NET_RAW))
        return -EPERM;

Root inside an unprivileged user namespace passes this check.

The same user can create a tun device in its own netns, since
tun_set_iff() checks ns_capable(net->user_ns, CAP_NET_ADMIN). It can also
set the device speed with ETHTOOL_SLINKSETTINGS:

- dev_ethtool() does the same ns_capable() check.
- ethtool_set_link_ksettings() does not validate the speed.
- tun_set_link_ksettings() copies the settings unchanged:

    memcpy(&tun->link_ksettings, cmd, sizeof(*cmd));

prb_calc_retire_blk_tmo() then reads that speed back through
__ethtool_get_link_ksettings() for the bound ifindex:

    unshare(CLONE_NEWUSER | CLONE_NEWNET)
      -> TUNSETIFF
      -> ETHTOOL_SLINKSETTINGS with speed = 1000 (or 2500)
      -> socket(AF_PACKET), bind to the tun device
      -> PACKET_VERSION = TPACKET_V3, PACKET_RX_RING

On systems that allow unprivileged user namespaces, this seems to let an
unprivileged local user start the maximum-rate soft hrtimer with no real
hardware. Would it be worth saying so in the commit message?

The same tun setup could also be used to test the fix at runtime. The
notes say the patch was only compile-tested because no 1 Gbps setup was
available.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923050101.1510064-1-zhangdairui%40gmail.com