Re: [PATCH net-next 1/2] nexthop: Fix out-of-bounds access during attribute validation

[Ido Schimmel <idosch@nvidia.com>]

On Sun, Mar 10, 2024 at 11:54:59AM -0600, David Ahern wrote:
> On 3/10/24 11:32 AM, Ido Schimmel wrote:
> > diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
> > index 5eb3ba568f4e..f3df80d2b980 100644
> > --- a/net/ipv4/nexthop.c
> > +++ b/net/ipv4/nexthop.c
> > @@ -3253,8 +3253,9 @@ static int rtm_del_nexthop(struct sk_buff *skb, struct nlmsghdr *nlh,
> >  	int err;
> >  	u32 id;
> >  
> > -	err = nlmsg_parse(nlh, sizeof(struct nhmsg), tb, NHA_MAX,
> > -			  rtm_nh_policy_del, extack);
> > +	err = nlmsg_parse(nlh, sizeof(struct nhmsg), tb,
> > +			  ARRAY_SIZE(rtm_nh_policy_del) - 1, rtm_nh_policy_del,
> 
> 'tb' on the stack only needs to be ARRAY_SIZE as well; that's the
> benefit of the approach - only declare what you need.

The reasoning for that is explained in Petr's commit message:

"
    - To allow querying for presence of the attribute, have all the attribute
      arrays sized to NHA_MAX, regardless of what is permitted by policy, and
      pass the corresponding value to nlmsg_parse() as well.
"

IOW, with resizing 'tb' to ARRAY_SIZE:

rtm_del_nexthop
    nh_valid_get_del_req
        if (tb[NHA_OP_FLAGS]) -> BOOM

However, I can add [1] and [2] as patches #1 and #2 and then squash [3]
into the current patch.

[1]
commit bf5184cc9a3596d3185c91f2f7986e7c6f2dba9c
Author: Ido Schimmel <idosch@nvidia.com>
Date:   Sun Mar 10 21:56:21 2024 +0200

    nexthop: Only parse NHA_OP_FLAGS for get messages that require it
    
    The attribute is parsed into 'op_flags' in nh_valid_get_del_req() which
    is called from the handlers of three message types: RTM_DELNEXTHOP,
    RTM_GETNEXTHOPBUCKET and RTM_GETNEXTHOP. The attribute is only used by
    the latter and rejected by the policies of the other two.
    
    Pass 'op_flags' as NULL from the handlers of the other two and only
    parse the attribute when the argument is not NULL.
    
    This is a preparation for a subsequent patch.
    
    Signed-off-by: Ido Schimmel <idosch@nvidia.com>

diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index 5eb3ba568f4e..03bacf9c0502 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -3229,10 +3229,12 @@ static int nh_valid_get_del_req(const struct nlmsghdr *nlh,
                return -EINVAL;
        }
 
-       if (tb[NHA_OP_FLAGS])
-               *op_flags = nla_get_u32(tb[NHA_OP_FLAGS]);
-       else
-               *op_flags = 0;
+       if (op_flags) {
+               if (tb[NHA_OP_FLAGS])
+                       *op_flags = nla_get_u32(tb[NHA_OP_FLAGS]);
+               else
+                       *op_flags = 0;
+       }
 
        return 0;
 }
@@ -3249,7 +3251,6 @@ static int rtm_del_nexthop(struct sk_buff *skb, struct nlmsghdr *nlh,
                .portid = NETLINK_CB(skb).portid,
        };
        struct nexthop *nh;
-       u32 op_flags;
        int err;
        u32 id;
 
@@ -3258,7 +3259,7 @@ static int rtm_del_nexthop(struct sk_buff *skb, struct nlmsghdr *nlh,
        if (err < 0)
                return err;
 
-       err = nh_valid_get_del_req(nlh, tb, &id, &op_flags, extack);
+       err = nh_valid_get_del_req(nlh, tb, &id, NULL, extack);
        if (err)
                return err;
 
@@ -3715,7 +3716,6 @@ static int nh_valid_get_bucket_req(const struct nlmsghdr *nlh,
                                   struct netlink_ext_ack *extack)
 {
        struct nlattr *tb[NHA_MAX + 1];
-       u32 op_flags;
        int err;
 
        err = nlmsg_parse(nlh, sizeof(struct nhmsg), tb, NHA_MAX,
@@ -3723,7 +3723,7 @@ static int nh_valid_get_bucket_req(const struct nlmsghdr *nlh,
        if (err < 0)
                return err;
 
-       err = nh_valid_get_del_req(nlh, tb, id, &op_flags, extack);
+       err = nh_valid_get_del_req(nlh, tb, id, NULL, extack);
        if (err)
                return err;

[2]
commit 585183403a6b692d71746527938b037f50feed65
Author: Ido Schimmel <idosch@nvidia.com>
Date:   Sun Mar 10 22:54:53 2024 +0200

    nexthop: Only parse NHA_OP_FLAGS for dump messages that require it
    
    The attribute is parsed in __nh_valid_dump_req() which is called by the
    dump handlers of RTM_GETNEXTHOP and RTM_GETNEXTHOPBUCKET although it is
    only used by the former and rejected by the policy of the latter.
    
    Move the parsing to nh_valid_dump_req() which is only called by the dump
    handler of RTM_GETNEXTHOP.
    
    This is a preparation for a subsequent patch.
    
    Signed-off-by: Ido Schimmel <idosch@nvidia.com>

diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index 03bacf9c0502..573da3660cb3 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -3397,11 +3397,6 @@ static int __nh_valid_dump_req(const struct nlmsghdr *nlh, struct nlattr **tb,
                return -EINVAL;
        }
 
-       if (tb[NHA_OP_FLAGS])
-               filter->op_flags = nla_get_u32(tb[NHA_OP_FLAGS]);
-       else
-               filter->op_flags = 0;
-
        return 0;
 }
 
@@ -3417,6 +3412,11 @@ static int nh_valid_dump_req(const struct nlmsghdr *nlh,
        if (err < 0)
                return err;
 
+       if (tb[NHA_OP_FLAGS])
+               filter->op_flags = nla_get_u32(tb[NHA_OP_FLAGS]);
+       else
+               filter->op_flags = 0;
+
        return __nh_valid_dump_req(nlh, tb, filter, cb->extack);
 }

[3]
diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index f6c9d834b989..0011b0076c5b 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -3243,8 +3243,8 @@ static int nh_valid_get_del_req(const struct nlmsghdr *nlh,
 static int rtm_del_nexthop(struct sk_buff *skb, struct nlmsghdr *nlh,
                           struct netlink_ext_ack *extack)
 {
+       struct nlattr *tb[ARRAY_SIZE(rtm_nh_policy_del)];
        struct net *net = sock_net(skb->sk);
-       struct nlattr *tb[NHA_MAX + 1];
        struct nl_info nlinfo = {
                .nlh = nlh,
                .nl_net = net,
@@ -3277,8 +3277,8 @@ static int rtm_del_nexthop(struct sk_buff *skb, struct nlmsghdr *nlh,
 static int rtm_get_nexthop(struct sk_buff *in_skb, struct nlmsghdr *nlh,
                           struct netlink_ext_ack *extack)
 {
+       struct nlattr *tb[ARRAY_SIZE(rtm_nh_policy_get)];
        struct net *net = sock_net(in_skb->sk);
-       struct nlattr *tb[NHA_MAX + 1];
        struct sk_buff *skb = NULL;
        struct nexthop *nh;
        u32 op_flags;
@@ -3406,7 +3406,7 @@ static int nh_valid_dump_req(const struct nlmsghdr *nlh,
                             struct nh_dump_filter *filter,
                             struct netlink_callback *cb)
 {
-       struct nlattr *tb[NHA_MAX + 1];
+       struct nlattr *tb[ARRAY_SIZE(rtm_nh_policy_dump)];
        int err;
 
        err = nlmsg_parse(nlh, sizeof(struct nhmsg), tb,
@@ -3550,7 +3550,7 @@ static int nh_valid_dump_bucket_req(const struct nlmsghdr *nlh,
                                    struct netlink_callback *cb)
 {
        struct nlattr *res_tb[ARRAY_SIZE(rtm_nh_res_bucket_policy_dump)];
-       struct nlattr *tb[NHA_MAX + 1];
+       struct nlattr *tb[ARRAY_SIZE(rtm_nh_policy_dump_bucket)];
        int err;
 
        err = nlmsg_parse(nlh, sizeof(struct nhmsg), tb,
@@ -3719,7 +3719,7 @@ static int nh_valid_get_bucket_req(const struct nlmsghdr *nlh,
                                   u32 *id, u16 *bucket_index,
                                   struct netlink_ext_ack *extack)
 {
-       struct nlattr *tb[NHA_MAX + 1];
+       struct nlattr *tb[ARRAY_SIZE(rtm_nh_policy_get_bucket)];
        int err;
 
        err = nlmsg_parse(nlh, sizeof(struct nhmsg), tb,