Re: [PATCH net] net: dcb: initialize PFC value before calling getpfccfg

[Daniel Machon]

> 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 <joasantonio108@xxxxxxxxx>
> 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