> dcbnl_getpfccfg() and dcbnl_cee_fill() pass the address of an
> uninitialized stack u8 to the driver's getpfccfg() op and then put it
> into the netlink reply unconditionally. The op returns void, so the
> core cannot tell whether the driver wrote the value.
>
> i40e_dcbnl_get_pfc_cfg() and ice_dcbnl_get_pfc_cfg() return without
> writing *setting unless the DCBX agent is host managed and in CEE mode.
> Both drivers normally run with firmware managed DCBX
> (DCB_CAP_DCBX_LLD_MANAGED), and i40e with FW LLDP disabled runs host
> managed in IEEE mode, so on a DCB capable i40e/ice port the PFC values
> in DCB_CMD_PFC_GCFG and DCB_CMD_CEE_GET replies, and in the CEE part of
> RTNLGRP_DCB notifications, come from uninitialized stack. The same u8
> is reused for all eight priorities, so this is one byte per reply.
>
> dcb_doit() only requires CAP_NET_ADMIN for RTM_SETDCB, so the GET
> commands are available to unprivileged users.
>
> Set the value to 0 before each call, like the PG config path already
> presets its outputs to DCB_ATTR_VALUE_UNDEFINED. 0 is also what the
> reply contains today on kernels built with CONFIG_INIT_STACK_ALL_ZERO,
> so nothing changes for those.
>
> Fixes: b94b013eb626 ("ice: Implement DCBNL support")
> Fixes: 5effa78e7c94 ("i40e: Add netlink callbacks support for software based
> DCB")
I am not entirely sure, but should this patch add a fixes tag for a driver, when
the change is only in the API?
Also, if this dates back to the introduction of DCBNL then I guess its not a
regression and maybe a fix for net-next?
> Signed-off-by: Joas Antonio dos Santos <[email protected]>
> Assisted-by: Yaga Agent HackerSec Claude:claude-opus-5-5
> ---
> Found by code inspection, compile-tested only (W=1). I don't have
> i40e/ice hardware to check the reply. On kernels built with
> CONFIG_INIT_STACK_ALL_ZERO the byte is already zero, so in practice
> it only leaks on builds without stack auto-initialization.
>
> net/dcb/dcbnl.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c
> index ab71f53128..ad0aebe0ad 100644
> --- a/net/dcb/dcbnl.c
> +++ b/net/dcb/dcbnl.c
> @@ -281,6 +281,7 @@ static int dcbnl_getpfccfg(struct net_device *netdev,
> struct nlmsghdr *nlh,
> if (!getall && !data[i])
> continue;
>
> + value = 0;
Why noy initialize this at the declaration, like getall is?
> netdev->dcbnl_ops->getpfccfg(netdev, i - DCB_PFC_UP_ATTR_0,
> &value);
> ret = nla_put_u8(skb, i, value);
> @@ -1395,6 +1396,7 @@ static int dcbnl_cee_fill(struct sk_buff *skb, struct
> net_device *netdev)
> goto nla_put_failure;
>
> for (i = DCB_PFC_UP_ATTR_0; i <= DCB_PFC_UP_ATTR_7; i++) {
> + value = 0;
Same.
> ops->getpfccfg(netdev, i - DCB_PFC_UP_ATTR_0, &value);
> if (nla_put_u8(skb, i, value))
> goto nla_put_failure;
/Daniel
