| Message ID | 20240311162307.545385-4-idosch@nvidia.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d8a21070b6e168d3800c2962a574f16020dd2951 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | nexthop: Fix two nexthop group statistics issues | expand |
On 3/11/24 10:23 AM, Ido Schimmel wrote: > 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> > --- > > Notes: > v2: > * Resize 'tb' using ARRAY_SIZE > > net/ipv4/nexthop.c | 29 ++++++++++++--------- > tools/testing/selftests/net/fib_nexthops.sh | 6 +++++ > 2 files changed, 23 insertions(+), 12 deletions(-) > Reviewed-by: David Ahern <dsahern@kernel.org> > 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" The basic() group of tests do not have a delete, so this is a good addition. However, the ipv6_fcnal and ipv4_fcnal do have a del - seems like those tests should have caught the out of bounds access.
On Mon, Mar 11, 2024 at 09:28:30PM -0600, David Ahern wrote: > On 3/11/24 10:23 AM, Ido Schimmel wrote: > Reviewed-by: David Ahern <dsahern@kernel.org> Thanks! > > > > 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" > > The basic() group of tests do not have a delete, so this is a good > addition. However, the ipv6_fcnal and ipv4_fcnal do have a del - seems > like those tests should have caught the out of bounds access. There are deletion tests, but they only provide the nexthop ID and the purpose of providing some bogus attribute ("group" in this case) was to trigger the out-of-bounds access in validate_nla(): pt = &policy[type]; As rtm_nh_policy_del does not contain an entry for NHA_GROUP.
diff --git a/net/ipv4/nexthop.c b/net/ipv4/nexthop.c index 573da3660cb3..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, @@ -3254,8 +3254,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; @@ -3276,16 +3277,17 @@ 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; 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; @@ -3404,10 +3406,11 @@ 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, 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; @@ -3547,10 +3550,11 @@ 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, 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; @@ -3715,10 +3719,11 @@ 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, 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"
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> --- Notes: v2: * Resize 'tb' using ARRAY_SIZE net/ipv4/nexthop.c | 29 ++++++++++++--------- tools/testing/selftests/net/fib_nexthops.sh | 6 +++++ 2 files changed, 23 insertions(+), 12 deletions(-)