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

[netdev-bot+sashiko@kernel.org]

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

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

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260910204612.3762015-1-edumazet%40google.com