Re: [PATCH 1/2] drbd: fix off-by-one in netlink NUL-string policy lengths

Am 02.08.26 um 13:05 schrieb Ionut Nechita:
> For NLA_NUL_STRING attributes, nla_policy.len is the maximum length of
> the string excluding the terminating NUL, as documented in
> include/net/netlink.h. The genl_magic infrastructure encoded that as
> 
> 	.len = maxlen - (nla_type == NLA_NUL_STRING)
> 
> but the hand-written policies that replaced it set .len to the full size
> of the destination buffer. Every NUL-string attribute therefore accepts
> a string that is one byte too long for the buffer it is copied into.
> 
> For such a string validate_nla() succeeds and nla_strscpy() truncates it
> and returns -E2BIG. The return value is stored without being checked in
> the corresponding unsigned length member, e.g.
> 
> 	s->cpu_mask_len = nla_strscpy(s->cpu_mask, nla, DRBD_CPU_MASK_SIZE);
> 
> set_resource_options() copies the whole struct into resource->res_opts,
> so a later dump of the resource options reaches res_opts_to_skb() with
> cpu_mask_len set to (__u32)-E2BIG. The payload length handed to
> nla_put() is computed there as
> 
> 	min_t(int, DRBD_CPU_MASK_SIZE,
> 	      s->cpu_mask_len + (s->cpu_mask_len < DRBD_CPU_MASK_SIZE))
> 
> which evaluates to a negative int. __nla_reserve() then stores
> nla_attr_size() of that negative value in the u16 nla_len, and
> __nla_put() calls memcpy() with an underflowed size argument.
> 
> The five net_conf algorithm names, both disk_conf device paths, the
> configuration context resource name and the resource cpu-mask are all
> affected. The shared secret is the one most likely to be hit in
> practice: SHARED_SECRET_MAX is 64, a secret of exactly 64 characters is
> what "openssl rand -hex 32" produces, and nla_put_status_info() feeds
> the unsanitized net_conf to net_conf_to_skb() for any caller holding
> CAP_SYS_ADMIN. The sanitized path clears both the secret and its length,
> so an unprivileged status dump is not affected.
> 
> Restore the bound the generated code used to emit. With the policy
> corrected nla_strscpy() can no longer truncate, so the unchecked return
> values become harmless again.
> 
> Reaching any of the affected attributes requires CAP_NET_ADMIN. Found by
> inspection while looking at the cpu-mask length limit; no user report.
> 
> Fixes: 8098eeb693c4 ("drbd: replace genl_magic with explicit netlink serialization")
> Signed-off-by: Ionut Nechita <[email protected]>
> ---
>  drivers/block/drbd/drbd_nl_gen.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)

Thanks for the patch, good catch. The fix looks correct.

Reviewed-by: Christoph Böhmwalder <[email protected]>

-- 
Christoph Böhmwalder
LINBIT | Keeping the Digital World Running
DRBD HA —  Disaster Recovery — Software defined Storage