Re: [PATCH iproute2] libnetlink: zero padding in rta_addattr_l()

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue,  3 Nov 2026 21:29:01 -0800
Dave Seddon <dave.seddon.ca@gmail.com> wrote:

> rta_addattr_l() copies alen bytes but advances the parent by
> RTA_ALIGN(len), so the 1-3 padding bytes are never written. Nests built
> in stack buffers (lwtunnel encap, RTA_MULTIPATH, RTA_METRICS) then send
> those bytes to the kernel. For example, in
> 
>   ip nexthop add id 53 encap mpls 101 ttl 10 via 192.0.2.1 dev d0
> 
> the three bytes after MPLS_IPTUNNEL_TTL are stack contents:
> 
>   05 00 02 00  0a 7f 00 00    MPLS_IPTUNNEL_TTL, ttl 10, stack bytes
>   05 00 02 00  0a 00 00 00    with this patch
> 
> and valgrind reports "sendmsg(msg.msg_iov[0]) points to uninitialised
> byte(s)".
> 
> The kernel skips padding when parsing, so nothing fails, but its own
> encoder zeroes it (__nla_reserve()). Do the same here.
> 
> Fixes: 6ef87f9cce21 ("ip: route: add congestion control metric")
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Dave Seddon <dave.seddon.ca@gmail.com>
> ---
> addattr_l() has the same gap, but all its callers zero their request,
> so it is left alone.
> 
> Kernel side: nla_next() advances by NLA_ALIGN(nla_len), so padding is
> never read; __nla_reserve() memsets it to zero.
> ---

Looks good but AI had some bike shedding on details.

Review summary: [PATCH iproute2] libnetlink: zero padding in rta_addattr_l()
Dave Seddon <dave.seddon.ca@gmail.com>, 2026-11-03
Patchwork-Id: 14942963  (delegate: dsahern@gmail.com)

VERDICT
  Fix is correct. Two commit-message problems. 

THE CHANGE
  lib/libnetlink.c, 1 insertion:
    +	memset((void *)subrta + len, 0, RTA_ALIGN(len) - len);

  rta_addattr_l() copies alen bytes but advances the parent by
  RTA_ALIGN(len), so 1-3 padding bytes were never written. Nests built
  in stack buffers sent stack contents to the kernel.

VERIFIED
  - Bounds safe: existing check reserves RTA_ALIGN(rta->rta_len) +
    RTA_ALIGN(len), so the memset cannot pass maxlen. Exact-fit case
    (maxlen=8, alen=1) returns -1 before reaching the memset.
  - alen == 0 (rta_nest path): len = RTA_LENGTH(0) = 4, already
    aligned, memset is a no-op, does not scribble past the header.
  - Byte pattern matches changelog exactly:
      before: 05 00 02 00  0a 7f 00 00
      after:  05 00 02 00  0a 00 00 00
  - Coverage complete: rta_addattr8/16/64 all route through
    rta_addattr_l(). rta_addattr32() needs nothing (len=8, aligned).
  - Applies to oss-nlseq tip (offset -7 lines), builds clean.
    Test apply reverted; working tree untouched.

PROBLEM 1 - note below the --- is wrong
  Claims "addattr_l() has the same gap". It does not. addattr_l() has
  zeroed its padding since 07f9436201eb (2005, "Use NLMSG_TAIL and add
  addraw_l") -- same memset, same file. That is the precedent this
  patch follows, worth citing rather than disclaiming.

PROBLEM 2 - Fixes tag is misdirected
  Fixes: 6ef87f9cce21 ("ip: route: add congestion control metric")
  is a real unaligned caller (RTAX_CC_ALGO string) but not the cause.
  The defect is in rta_addattr_l() itself, back to the BitKeeper
  import (aba5acdf, 2004) -- no useful commit to tag. The MPLS TTL
  reproducer in the changelog is from 9688cf3b, different again.
  Recommend: drop the tag, state in the body that this affects every
  caller passing an unaligned length.

SEVERITY
  Hygiene, not a vulnerability. Kernel nla_next() advances by
  NLA_ALIGN(nla_len) so padding is never parsed; the 1-3 bytes go to
  the kernel, not onto the wire. Real motivations are the valgrind
  report and matching the kernel's own __nla_reserve(). The changelog
  already frames this accurately.