Re: [PATCH net 4/4] drop_monitor: fix out-of-bounds write in reset_per_cpu_data()

On Fri, Sep 11, 2026 at 2:08 PM [off-list ref] wrote:

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

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

- [Medium] Commit message overstates the impact: the pre-patch memset()
  in reset_per_cpu_data() (net/core/drop_monitor.c:165) is NOT an out-…

--

drop_monitor: fix out-of-bounds write in reset_per_cpu_data()

The patch stops adding sizeof(struct nlattr) to al in
reset_per_cpu_data(), and instead allocates the skb with
genlmsg_new(nla_total_size(al), GFP_KERNEL) so that al describes only the
attribute payload that is later zeroed via memset(msg, 0, al).

quoted

diff --git a/net/core/drop_monitor.c b/net/core/drop_monitor.c
index 795c15dd1771a..edc660778408e 100644
--- a/net/core/drop_monitor.c
+++ b/net/core/drop_monitor.c

@@ -141,9 +141,8 @@ static struct sk_buff *reset_per_cpu_data(struct per_cpu_dm_data *data)

      al = sizeof(struct net_dm_alert_msg);
      al += dm_hit_limit * sizeof(struct net_dm_drop_point);
-     al += sizeof(struct nlattr);

-     skb = genlmsg_new(al, GFP_KERNEL);
+     skb = genlmsg_new(nla_total_size(al), GFP_KERNEL);

[Severity: Medium]
Is the impact described in the changelog accurate?  The subject says "fix
out-of-bounds write", and the body says the memset() is

    exceeding the allocated buffer by sizeof(struct nlattr) (4 bytes) and
    corrupting skb_shared_info.

With the in-tree constants, dm_hit_limit is a fixed "static int
dm_hit_limit = 64" with no module_param and no way for userspace to change
it, sizeof(struct net_dm_alert_msg) is 4 and sizeof(struct
net_dm_drop_point) is 12, so the old al was 4 + 768 + 4 = 776.

That gives genlmsg_new(776) -> nlmsg_new(780) -> alloc_skb(796), and with
nlmsghdr [0,16), genlmsghdr [16,20) and the nlattr header [20,24), msg sits
at offset 24 and memset(msg, 0, 776) writes up to offset 800, i.e. 4 bytes
past the 796 bytes that were requested.

But the head buffer that gets allocated is larger than the request, and
skb_shared_info is placed at the end of the rounded-up zone:

net/core/skbuff.c:kmalloc_reserve() {
        ...
        obj_size = SKB_HEAD_ALIGN(*size);
        ...
        obj_size = kmalloc_size_roundup(obj_size);
        *size = (unsigned int)obj_size;
}

net/core/skbuff.c:__alloc_skb() {
        ...
        /* kmalloc_size_roundup() might give us more room than requested.
         * Put skb_shared_info exactly at the end of allocated zone,
         * to allow max possible filling before reallocation.
         */
        __finalize_skb_around(skb, data, size);
}

For a 796-byte request SKB_HEAD_ALIGN(796) is 832 + 320 = 1152, which
kmalloc_size_roundup() rounds to 2048, so skb->end lands near offset 1728
and the 4 extra zeroed bytes fall in unused tailroom.  KASAN would also not
report anything, since kmalloc is called with the already-rounded size.  The
small-head-cache path gives *size = SKB_SMALL_HEAD_CACHE_SIZE, which is also
comfortably above 800.

Also, nla_total_size(772) is 776, exactly the previous genlmsg_new()
argument, so the allocation size is unchanged by this patch; only the memset
length shrinks from 776 to 772, which matches the usable payload (796 - 24 =
772 = 4 + 64 * 12) and the fill limit enforced in trace_drop_common().

The code change itself looks correct, but with the Fixes: 683703a26e46 tag
this will be picked up for stable and CVE triage as an out-of-bounds write.
Could the changelog be reworded to describe it as a size-accounting cleanup,
and to drop the claim about corrupting skb_shared_info?

This feedback is quite shocking and annoying.

Do I need to double check everything and make precise computations?

No, I do not.