diff mbox series

[net,v1] net: dcb: choose correct policy to parse DCB_ATTR_BCN

Message ID 20230731045216.3779420-1-linma@zju.edu.cn
State New
Headers
Series [net,v1] net: dcb: choose correct policy to parse DCB_ATTR_BCN |

Commit Message

Lin Ma July 31, 2023, 4:52 a.m. UTC 
  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>
---
 net/dcb/dcbnl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Simon Horman July 31, 2023, 10 a.m. UTC | #1 
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>
Lin Ma Aug. 1, 2023, 1:34 a.m. UTC | #2 
Hello Markus,

> 
> …
> > This patch use correct dcbnl_bcn_nest policy to parse the
> > tb[DCB_ATTR_BCN] nested TLV.
> 
> Are imperative change descriptions still preferred?
> 
> See also:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.5-rc3#n94
> 
> Regards,
> Markus

Yeah, thanks for reminding me. I haven't been paying attention to that and I will remember this ever since. :D

Regards,
Lin
Dan Carpenter Aug. 1, 2023, 5:46 a.m. UTC | #3 
On Tue, Aug 01, 2023 at 09:34:17AM +0800, Lin Ma wrote:
> Hello Markus,
> 
> > 
> > …
> > > This patch use correct dcbnl_bcn_nest policy to parse the
> > > tb[DCB_ATTR_BCN] nested TLV.
> > 
> > Are imperative change descriptions still preferred?
> > 
> > See also:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.5-rc3#n94
> > 
> > Regards,
> > Markus
> 
> Yeah, thanks for reminding me. I haven't been paying attention to that
> and I will remember this ever since. :D

Simon reviewed the patch already.  Don't listen to Markus.  He's banned
from vger.

https://lore.kernel.org/all/2023073123-poser-panhandle-1cb7@gregkh/

regards,
dan carpenter
Lin Ma Aug. 1, 2023, 6:01 a.m. UTC | #4 
Hello Dan,

> 
> Simon reviewed the patch already.  Don't listen to Markus.  He's banned
> from vger.
> 
> https://lore.kernel.org/all/2023073123-poser-panhandle-1cb7@gregkh/
> 
> regards,
> dan carpenter

Ooooops, I never thought of it like this. I will take note of that :).

Thanks
Lin
Dan Carpenter Aug. 1, 2023, 10:28 a.m. UTC | #5 
I'm not trying to be mean.  I don't go out looking for Markus's emails,
but he often adds kernel-janitors to the CC list.  Kernel janitors is a
vger list so he's banned but we still see the responses to his emails.

In recent months probably seven maintainers have asked him over and over
(maybe 20 times?) to stop with this nonsense.  So he knew he shouldn't
have asked Lin Ma to redo the patch.

regards,
dan carpenter
diff mbox series

Patch

diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c
index c0c438128575..2e6b8c8fd2de 100644
--- a/net/dcb/dcbnl.c
+++ b/net/dcb/dcbnl.c
@@ -980,7 +980,7 @@  static int dcbnl_bcn_setcfg(struct net_device *netdev, struct nlmsghdr *nlh,
 		return -EOPNOTSUPP;
 
 	ret = nla_parse_nested_deprecated(data, DCB_BCN_ATTR_MAX,
-					  tb[DCB_ATTR_BCN], dcbnl_pfc_up_nest,
+					  tb[DCB_ATTR_BCN], dcbnl_bcn_nest,
 					  NULL);
 	if (ret)
 		return ret;