* [PATCH net-next 0/2] nexthop: Fix two nexthop group statistics issues
@ 2024-03-10 17:32 Ido Schimmel
2024-03-10 17:32 ` [PATCH net-next 1/2] nexthop: Fix out-of-bounds access during attribute validation Ido Schimmel
2024-03-10 17:32 ` [PATCH net-next 2/2] nexthop: Fix splat with CONFIG_DEBUG_PREEMPT=y Ido Schimmel
0 siblings, 2 replies; 9+ messages in thread
From: Ido Schimmel @ 2024-03-10 17:32 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, pabeni, edumazet, petrm, dsahern, Ido Schimmel
Fix two issues that were introduced as part of the recent nexthop group
statistics submission. See the commit messages for more details.
Ido Schimmel (2):
nexthop: Fix out-of-bounds access during attribute validation
nexthop: Fix splat with CONFIG_DEBUG_PREEMPT=y
net/ipv4/nexthop.c | 22 +++++++++++++--------
tools/testing/selftests/net/fib_nexthops.sh | 6 ++++++
2 files changed, 20 insertions(+), 8 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH net-next 1/2] nexthop: Fix out-of-bounds access during attribute validation
2024-03-10 17:32 [PATCH net-next 0/2] nexthop: Fix two nexthop group statistics issues Ido Schimmel
@ 2024-03-10 17:32 ` Ido Schimmel
2024-03-10 17:54 ` David Ahern
2024-03-10 17:32 ` [PATCH net-next 2/2] nexthop: Fix splat with CONFIG_DEBUG_PREEMPT=y Ido Schimmel
1 sibling, 1 reply; 9+ messages in thread
From: Ido Schimmel @ 2024-03-10 17:32 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, pabeni, edumazet, petrm, dsahern, Ido Schimmel
Passing a maximum attribute type to nlmsg_parse() that is larger than
the size of the passed policy will result in an out-of-bounds access [1]
when the attribute type is used as an index into the policy array.
Fix by setting the maximum attribute type according to the policy size,
as is already done for RTM_NEWNEXTHOP messages. Add a test case that
triggers the bug.
No regressions in fib nexthops tests:
# ./fib_nexthops.sh
[...]
Tests passed: 236
Tests failed: 0
[1]
BUG: KASAN: global-out-of-bounds in __nla_validate_parse+0x1e53/0x2940
Read of size 1 at addr ffffffff99ab4d20 by task ip/610
CPU: 3 PID: 610 Comm: ip Not tainted 6.8.0-rc7-custom-gd435d6e3e161 #9
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-1.fc38 04/01/2014
Call Trace:
<TASK>
dump_stack_lvl+0x8f/0xe0
print_report+0xcf/0x670
kasan_report+0xd8/0x110
__nla_validate_parse+0x1e53/0x2940
__nla_parse+0x40/0x50
rtm_del_nexthop+0x1bd/0x400
rtnetlink_rcv_msg+0x3cc/0xf20
netlink_rcv_skb+0x170/0x440
netlink_unicast+0x540/0x820
netlink_sendmsg+0x8d3/0xdb0
____sys_sendmsg+0x31f/0xa60
___sys_sendmsg+0x13a/0x1e0
__sys_sendmsg+0x11c/0x1f0
do_syscall_64+0xc5/0x1d0
entry_SYSCALL_64_after_hwframe+0x63/0x6b
[...]
The buggy address belongs to the variable:
rtm_nh_policy_del+0x20/0x40
Fixes: 2118f9390d83 ("net: nexthop: Adjust netlink policy parsing for a new attribute")
Reported-by: Eric Dumazet <edumazet@google.com>
Closes: https://lore.kernel.org/netdev/CANn89i+UNcG0PJMW5X7gOMunF38ryMh=L1aeZUKH3kL4UdUqag@mail.gmail.com/
Reported-by: syzbot+65bb09a7208ce3d4a633@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/netdev/00000000000088981b06133bc07b@google.com/
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
net/ipv4/nexthop.c | 19 ++++++++++++-------
tools/testing/selftests/net/fib_nexthops.sh | 6 ++++++
2 files changed, 18 insertions(+), 7 deletions(-)
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,
+ extack);
if (err < 0)
return err;
@@ -3283,8 +3284,9 @@ static int rtm_get_nexthop(struct sk_buff *in_skb, struct nlmsghdr *nlh,
int err;
u32 id;
- err = nlmsg_parse(nlh, sizeof(struct nhmsg), tb, NHA_MAX,
- rtm_nh_policy_get, extack);
+ err = nlmsg_parse(nlh, sizeof(struct nhmsg), tb,
+ ARRAY_SIZE(rtm_nh_policy_get) - 1, rtm_nh_policy_get,
+ extack);
if (err < 0)
return err;
@@ -3411,7 +3413,8 @@ static int nh_valid_dump_req(const struct nlmsghdr *nlh,
struct nlattr *tb[NHA_MAX + 1];
int err;
- err = nlmsg_parse(nlh, sizeof(struct nhmsg), tb, NHA_MAX,
+ err = nlmsg_parse(nlh, sizeof(struct nhmsg), tb,
+ ARRAY_SIZE(rtm_nh_policy_dump) - 1,
rtm_nh_policy_dump, cb->extack);
if (err < 0)
return err;
@@ -3549,7 +3552,8 @@ static int nh_valid_dump_bucket_req(const struct nlmsghdr *nlh,
struct nlattr *tb[NHA_MAX + 1];
int err;
- err = nlmsg_parse(nlh, sizeof(struct nhmsg), tb, NHA_MAX,
+ err = nlmsg_parse(nlh, sizeof(struct nhmsg), tb,
+ ARRAY_SIZE(rtm_nh_policy_dump_bucket) - 1,
rtm_nh_policy_dump_bucket, NULL);
if (err < 0)
return err;
@@ -3718,7 +3722,8 @@ static int nh_valid_get_bucket_req(const struct nlmsghdr *nlh,
u32 op_flags;
int err;
- err = nlmsg_parse(nlh, sizeof(struct nhmsg), tb, NHA_MAX,
+ err = nlmsg_parse(nlh, sizeof(struct nhmsg), tb,
+ ARRAY_SIZE(rtm_nh_policy_get_bucket) - 1,
rtm_nh_policy_get_bucket, extack);
if (err < 0)
return err;
diff --git a/tools/testing/selftests/net/fib_nexthops.sh b/tools/testing/selftests/net/fib_nexthops.sh
index d5a281aadbac..ac0b2c6a5761 100755
--- a/tools/testing/selftests/net/fib_nexthops.sh
+++ b/tools/testing/selftests/net/fib_nexthops.sh
@@ -2066,6 +2066,12 @@ basic()
run_cmd "$IP nexthop get id 1"
log_test $? 2 "Nexthop get on non-existent id"
+ run_cmd "$IP nexthop del id 1"
+ log_test $? 2 "Nexthop del with non-existent id"
+
+ run_cmd "$IP nexthop del id 1 group 1/2/3/4/5/6/7/8"
+ log_test $? 2 "Nexthop del with non-existent id and extra attributes"
+
# attempt to create nh without a device or gw - fails
run_cmd "$IP nexthop add id 1"
log_test $? 2 "Nexthop with no device or gateway"
--
2.43.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH net-next 2/2] nexthop: Fix splat with CONFIG_DEBUG_PREEMPT=y
2024-03-10 17:32 [PATCH net-next 0/2] nexthop: Fix two nexthop group statistics issues Ido Schimmel
2024-03-10 17:32 ` [PATCH net-next 1/2] nexthop: Fix out-of-bounds access during attribute validation Ido Schimmel
@ 2024-03-10 17:32 ` Ido Schimmel
1 sibling, 0 replies; 9+ messages in thread
From: Ido Schimmel @ 2024-03-10 17:32 UTC (permalink / raw)
To: netdev; +Cc: davem, kuba, pabeni, edumazet, petrm, dsahern, Ido Schimmel
Locally generated packets can increment the new nexthop statistics from
process context, resulting in the following splat [1] due to preemption
being enabled. Fix by using get_cpu_ptr() / put_cpu_ptr() which will
which take care of disabling / enabling preemption.
BUG: using smp_processor_id() in preemptible [00000000] code: ping/949
caller is nexthop_select_path+0xcf8/0x1e30
CPU: 12 PID: 949 Comm: ping Not tainted 6.8.0-rc7-custom-gcb450f605fae #11
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-1.fc38 04/01/2014
Call Trace:
<TASK>
dump_stack_lvl+0xbd/0xe0
check_preemption_disabled+0xce/0xe0
nexthop_select_path+0xcf8/0x1e30
fib_select_multipath+0x865/0x18b0
fib_select_path+0x311/0x1160
ip_route_output_key_hash_rcu+0xe54/0x2720
ip_route_output_key_hash+0x193/0x380
ip_route_output_flow+0x25/0x130
raw_sendmsg+0xbab/0x34a0
inet_sendmsg+0xa2/0xe0
__sys_sendto+0x2ad/0x430
__x64_sys_sendto+0xe5/0x1c0
do_syscall_64+0xc5/0x1d0
entry_SYSCALL_64_after_hwframe+0x63/0x6b
[...]
Fixes: f4676ea74b85 ("net: nexthop: Add nexthop group entry stats")
Signed-off-by: Ido Schimmel <idosch@nvidia.com>
---
net/ipv4/nexthop.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c
index f3df80d2b980..fe5531f1b39f 100644
--- a/net/ipv4/nexthop.c
+++ b/net/ipv4/nexthop.c
@@ -673,10 +673,11 @@ static void nh_grp_entry_stats_inc(struct nh_grp_entry *nhge)
{
struct nh_grp_entry_stats *cpu_stats;
- cpu_stats = this_cpu_ptr(nhge->stats);
+ cpu_stats = get_cpu_ptr(nhge->stats);
u64_stats_update_begin(&cpu_stats->syncp);
u64_stats_inc(&cpu_stats->packets);
u64_stats_update_end(&cpu_stats->syncp);
+ put_cpu_ptr(cpu_stats);
}
static void nh_grp_entry_stats_read(struct nh_grp_entry *nhge,
--
2.43.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 1/2] nexthop: Fix out-of-bounds access during attribute validation
2024-03-10 17:32 ` [PATCH net-next 1/2] nexthop: Fix out-of-bounds access during attribute validation Ido Schimmel
@ 2024-03-10 17:54 ` David Ahern
2024-03-10 21:41 ` Ido Schimmel
0 siblings, 1 reply; 9+ messages in thread
From: David Ahern @ 2024-03-10 17:54 UTC (permalink / raw)
To: Ido Schimmel, netdev; +Cc: davem, kuba, pabeni, edumazet, petrm
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.
Comment applies to the other locations as well.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 1/2] nexthop: Fix out-of-bounds access during attribute validation
2024-03-10 17:54 ` David Ahern
@ 2024-03-10 21:41 ` Ido Schimmel
2024-03-11 1:10 ` David Ahern
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Ido Schimmel @ 2024-03-10 21:41 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, davem, kuba, pabeni, edumazet, petrm
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,
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 1/2] nexthop: Fix out-of-bounds access during attribute validation
2024-03-10 21:41 ` Ido Schimmel
@ 2024-03-11 1:10 ` David Ahern
2024-03-11 10:27 ` Petr Machata
2024-03-11 14:47 ` Jakub Kicinski
2 siblings, 0 replies; 9+ messages in thread
From: David Ahern @ 2024-03-11 1:10 UTC (permalink / raw)
To: Ido Schimmel; +Cc: netdev, davem, kuba, pabeni, edumazet, petrm
On 3/10/24 3:41 PM, Ido Schimmel wrote:
> However, I can add [1] and [2] as patches #1 and #2 and then squash [3]
> into the current patch.
yes, please. Thank you for the fixups.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 1/2] nexthop: Fix out-of-bounds access during attribute validation
2024-03-10 21:41 ` Ido Schimmel
2024-03-11 1:10 ` David Ahern
@ 2024-03-11 10:27 ` Petr Machata
2024-03-11 14:47 ` Jakub Kicinski
2 siblings, 0 replies; 9+ messages in thread
From: Petr Machata @ 2024-03-11 10:27 UTC (permalink / raw)
To: Ido Schimmel; +Cc: David Ahern, netdev, davem, kuba, pabeni, edumazet, petrm
Ido Schimmel <idosch@nvidia.com> writes:
> 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
Yep. I passed NHA_MAX to nlmsg_parse to get the tb array properly
initialized, but missed the obvious fact that it will then expect the
policy arrays to be this long as well. Oops.
One way would be to just initialize the arrays:
modified net/ipv4/nexthop.c
@@ -3243,7 +3243,7 @@ static int rtm_del_nexthop(struct sk_buff *skb, struct nlmsghdr *nlh,
struct netlink_ext_ack *extack)
{
struct net *net = sock_net(skb->sk);
- struct nlattr *tb[NHA_MAX + 1];
+ struct nlattr *tb[NHA_MAX + 1] = {};
struct nl_info nlinfo = {
.nlh = nlh,
.nl_net = net,
But what you propose below looks OK to me as well, and conserves the
stack space. (Until the policies grow again anyway.)
> 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,
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 1/2] nexthop: Fix out-of-bounds access during attribute validation
2024-03-10 21:41 ` Ido Schimmel
2024-03-11 1:10 ` David Ahern
2024-03-11 10:27 ` Petr Machata
@ 2024-03-11 14:47 ` Jakub Kicinski
2024-03-11 15:52 ` Petr Machata
2 siblings, 1 reply; 9+ messages in thread
From: Jakub Kicinski @ 2024-03-11 14:47 UTC (permalink / raw)
To: Ido Schimmel; +Cc: David Ahern, netdev, davem, pabeni, edumazet, petrm
On Sun, 10 Mar 2024 23:41:53 +0200 Ido Schimmel wrote:
> > '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
v2 coming, right? Please repost as soon as possible. v1 crashes AFAICT
because tb is not 0-initialized so tb[NHA_OP_FLAGS] reads garbage.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net-next 1/2] nexthop: Fix out-of-bounds access during attribute validation
2024-03-11 14:47 ` Jakub Kicinski
@ 2024-03-11 15:52 ` Petr Machata
0 siblings, 0 replies; 9+ messages in thread
From: Petr Machata @ 2024-03-11 15:52 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Ido Schimmel, David Ahern, netdev, davem, pabeni, edumazet, petrm
Jakub Kicinski <kuba@kernel.org> writes:
> On Sun, 10 Mar 2024 23:41:53 +0200 Ido Schimmel wrote:
>> > '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
>
> v2 coming, right? Please repost as soon as possible. v1 crashes AFAICT
> because tb is not 0-initialized so tb[NHA_OP_FLAGS] reads garbage.
Ido will send v2 later today.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-03-11 15:53 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-10 17:32 [PATCH net-next 0/2] nexthop: Fix two nexthop group statistics issues Ido Schimmel
2024-03-10 17:32 ` [PATCH net-next 1/2] nexthop: Fix out-of-bounds access during attribute validation Ido Schimmel
2024-03-10 17:54 ` David Ahern
2024-03-10 21:41 ` Ido Schimmel
2024-03-11 1:10 ` David Ahern
2024-03-11 10:27 ` Petr Machata
2024-03-11 14:47 ` Jakub Kicinski
2024-03-11 15:52 ` Petr Machata
2024-03-10 17:32 ` [PATCH net-next 2/2] nexthop: Fix splat with CONFIG_DEBUG_PREEMPT=y Ido Schimmel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).