Messages in this thread | | | Date | Tue, 27 Feb 2024 13:25:35 +0200 | Subject | Re: [PATCH net v1] rtnetlink: fix error logic of IFLA_BRIDGE_FLAGS writing back | From | Nikolay Aleksandrov <> |
| |
On 2/27/24 13:01, Lin Ma wrote: > In the commit d73ef2d69c0d ("rtnetlink: let rtnl_bridge_setlink checks > IFLA_BRIDGE_MODE length"), an adjustment was made to the old loop logic > in the function `rtnl_bridge_setlink` to enable the loop to also check > the length of the IFLA_BRIDGE_MODE attribute. However, this adjustment > removed the `break` statement and led to an error logic of the flags > writing back at the end of this function. > > if (have_flags) > memcpy(nla_data(attr), &flags, sizeof(flags)); > // attr should point to IFLA_BRIDGE_FLAGS NLA !!! > > Before the mentioned commit, the `attr` is granted to be IFLA_BRIDGE_FLAGS. > However, this is not necessarily true fow now as the updated loop will let > the attr point to the last NLA, even an invalid NLA which could cause > overflow writes. > > This patch introduces a new variable `br_flag` to save the NLA pointer > that points to IFLA_BRIDGE_FLAGS and uses it to resolve the mentioned > error logic. > > Fixes: d73ef2d69c0d ("rtnetlink: let rtnl_bridge_setlink checks IFLA_BRIDGE_MODE length") > Signed-off-by: Lin Ma <linma@zju.edu.cn> > ---
That fix is obviously broken, I don't know how I missed it back then. One comment below,
> net/core/rtnetlink.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > index 9c4f427f3a50..e9f16e5e3515 100644 > --- a/net/core/rtnetlink.c > +++ b/net/core/rtnetlink.c > @@ -5169,10 +5169,9 @@ static int rtnl_bridge_setlink(struct sk_buff *skb, struct nlmsghdr *nlh, > struct net *net = sock_net(skb->sk); > struct ifinfomsg *ifm; > struct net_device *dev; > - struct nlattr *br_spec, *attr = NULL; > + struct nlattr *br_spec, *attr, *br_flag = NULL;
Please name the variable to something that describes it better, like br_flags_attr.
> int rem, err = -EOPNOTSUPP; > u16 flags = 0; > - bool have_flags = false; > > if (nlmsg_len(nlh) < sizeof(*ifm)) > return -EINVAL; > @@ -5190,11 +5189,11 @@ static int rtnl_bridge_setlink(struct sk_buff *skb, struct nlmsghdr *nlh, > br_spec = nlmsg_find_attr(nlh, sizeof(struct ifinfomsg), IFLA_AF_SPEC); > if (br_spec) { > nla_for_each_nested(attr, br_spec, rem) { > - if (nla_type(attr) == IFLA_BRIDGE_FLAGS && !have_flags) { > + if (nla_type(attr) == IFLA_BRIDGE_FLAGS && !br_flag) { > if (nla_len(attr) < sizeof(flags)) > return -EINVAL; > > - have_flags = true; > + br_flag = attr; > flags = nla_get_u16(attr); > } > > @@ -5238,8 +5237,8 @@ static int rtnl_bridge_setlink(struct sk_buff *skb, struct nlmsghdr *nlh, > } > } > > - if (have_flags) > - memcpy(nla_data(attr), &flags, sizeof(flags)); > + if (br_flag) > + memcpy(nla_data(br_flag), &flags, sizeof(flags)); > out: > return err; > }
| |