| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Ze4pIe_E4BgkCP6w@shredder>
Date: Sun, 10 Mar 2024 23:41:53 +0200
From: Ido Schimmel <idosch@...dia.com>
To: David Ahern <dsahern@...nel.org>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
pabeni@...hat.com, edumazet@...gle.com, petrm@...dia.com
Subject: Re: [PATCH net-next 1/2] nexthop: Fix out-of-bounds access during
attribute validation
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@...dia.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@...dia.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@...dia.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@...dia.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,
Powered by blists - more mailing lists