Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 31 Jul 2023 12:00:26 +0200
From:   Simon Horman <horms@kernel.org>
To:     Lin Ma <linma@zju.edu.cn>
Cc:     davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, daniel.machon@microchip.com, petrm@nvidia.com,
        peter.p.waskiewicz.jr@intel.com, jeffrey.t.kirsher@intel.com,
        alexander.h.duyck@intel.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net v1] net: dcb: choose correct policy to parse
 DCB_ATTR_BCN
Message-ID: <ZMeGOiANXBqDTBWm@kernel.org>
References: <20230731045216.3779420-1-linma@zju.edu.cn>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230731045216.3779420-1-linma@zju.edu.cn>
Precedence: bulk

On Mon, Jul 31, 2023 at 12:52:16PM +0800, Lin Ma wrote:
> The dcbnl_bcn_setcfg uses erroneous policy to parse tb[DCB_ATTR_BCN],
> which is introduced in commit 859ee3c43812 ("DCB: Add support for DCB
> BCN"). Please see the comment in below code
> 
> static int dcbnl_bcn_setcfg(...)
> {
>   ...
>   ret = nla_parse_nested_deprecated(..., dcbnl_pfc_up_nest, .. )
>   // !!! dcbnl_pfc_up_nest for attributes
>   //  DCB_PFC_UP_ATTR_0 to DCB_PFC_UP_ATTR_ALL in enum dcbnl_pfc_up_attrs
>   ...
>   for (i = DCB_BCN_ATTR_RP_0; i <= DCB_BCN_ATTR_RP_7; i++) {
>   // !!! DCB_BCN_ATTR_RP_0 to DCB_BCN_ATTR_RP_7 in enum dcbnl_bcn_attrs
>     ...
>     value_byte = nla_get_u8(data[i]);
>     ...
>   }
>   ...
>   for (i = DCB_BCN_ATTR_BCNA_0; i <= DCB_BCN_ATTR_RI; i++) {
>   // !!! DCB_BCN_ATTR_BCNA_0 to DCB_BCN_ATTR_RI in enum dcbnl_bcn_attrs
>   ...
>     value_int = nla_get_u32(data[i]);
>   ...
>   }
>   ...
> }
> 
> That is, the nla_parse_nested_deprecated uses dcbnl_pfc_up_nest
> attributes to parse nlattr defined in dcbnl_pfc_up_attrs. But the
> following access code fetch each nlattr as dcbnl_bcn_attrs attributes.
> By looking up the associated nla_policy for dcbnl_bcn_attrs. We can find
> the beginning part of these two policies are "same".
> 
> static const struct nla_policy dcbnl_pfc_up_nest[...] = {
> 	[DCB_PFC_UP_ATTR_0]   = {.type = NLA_U8},
> 	[DCB_PFC_UP_ATTR_1]   = {.type = NLA_U8},
> 	[DCB_PFC_UP_ATTR_2]   = {.type = NLA_U8},
> 	[DCB_PFC_UP_ATTR_3]   = {.type = NLA_U8},
> 	[DCB_PFC_UP_ATTR_4]   = {.type = NLA_U8},
> 	[DCB_PFC_UP_ATTR_5]   = {.type = NLA_U8},
> 	[DCB_PFC_UP_ATTR_6]   = {.type = NLA_U8},
> 	[DCB_PFC_UP_ATTR_7]   = {.type = NLA_U8},
> 	[DCB_PFC_UP_ATTR_ALL] = {.type = NLA_FLAG},
> };
> 
> static const struct nla_policy dcbnl_bcn_nest[...] = {
> 	[DCB_BCN_ATTR_RP_0]         = {.type = NLA_U8},
> 	[DCB_BCN_ATTR_RP_1]         = {.type = NLA_U8},
> 	[DCB_BCN_ATTR_RP_2]         = {.type = NLA_U8},
> 	[DCB_BCN_ATTR_RP_3]         = {.type = NLA_U8},
> 	[DCB_BCN_ATTR_RP_4]         = {.type = NLA_U8},
> 	[DCB_BCN_ATTR_RP_5]         = {.type = NLA_U8},
> 	[DCB_BCN_ATTR_RP_6]         = {.type = NLA_U8},
> 	[DCB_BCN_ATTR_RP_7]         = {.type = NLA_U8},
> 	[DCB_BCN_ATTR_RP_ALL]       = {.type = NLA_FLAG},
> 	// from here is somewhat different
> 	[DCB_BCN_ATTR_BCNA_0]       = {.type = NLA_U32},
>         ...
>         [DCB_BCN_ATTR_ALL]          = {.type = NLA_FLAG},
> };
> 
> Therefore, the current code is buggy and this
> nla_parse_nested_deprecated could overflow the dcbnl_pfc_up_nest and use
> the adjacent nla_policy to parse attributes from DCB_BCN_ATTR_BCNA_0.
> 
> This patch use correct dcbnl_bcn_nest policy to parse the
> tb[DCB_ATTR_BCN] nested TLV.
> 
> Fixes: 859ee3c43812 ("DCB: Add support for DCB BCN")
> Signed-off-by: Lin Ma <linma@zju.edu.cn>

Reviewed-by: Simon Horman <horms@kernel.org>