linux/net/openvswitch/meter.c

771 lines
18 KiB

net: openvswitch: fix potential UAF on meter attach failure While attaching a newly created meter attach_meter() function makes the new meter visible to other CPUs but can still fail afterwards. On failure, it detaches the meter back and returns an error. However, this is an unexpected behavior for the ovs_meter_cmd_set() that uses a plain kfree(meter) on attach failure without waiting for RCU readers to stop using it, assuming it was never visible. This is never a problem for ovs-vswitchd as it always creates meters before creating any flows that use them. But the UAF can be triggered with a custom application using uAPI: BUG: KASAN: slab-use-after-free in ovs_meter_execute (net/openvswitch/meter.c:653) Read of size 8 at addr ffff88810d152650 by task meter/2508 Call Trace: ovs_meter_execute (net/openvswitch/meter.c:653) do_execute_actions (net/openvswitch/actions.c:1407) ovs_execute_actions (net/openvswitch/actions.c:1584) ovs_packet_cmd_execute (net/openvswitch/datapath.c:703) ... netlink_sendmsg (af_netlink.c:1900) Allocated by task 2519: __kasan_kmalloc (mm/kasan/common.c:398 mm/kasan/common.c:415) ovs_meter_cmd_set (net/openvswitch/meter.c:422) ... netlink_sendmsg (af_netlink.c:1900) Freed by task 2519: kfree (mm/slub.c:2705 mm/slub.c:6405 mm/slub.c:6720) ovs_meter_cmd_set (net/openvswitch/meter.c:479) ... netlink_sendmsg (af_netlink.c:1900) Fix that by making sure attach_meter() doesn't make the meter visible until all the checks are done and the function can't fail anymore. This also makes sure the "hash" value is calculated after the potential re-sizing of the table. Reported by Trend Micro's Zero Day Initiative as ZDI-CAN-31642. Fixes: c7c4c44c9a95 ("net: openvswitch: expand the meters supported number") Cc: stable@vger.kernel.org Signed-off-by: Ilya Maximets <i.maximets@ovn.org> Reviewed-by: Eelco Chaudron <echaudro@redhat.com> Link: https://patch.msgid.link/20260727121022.198461-1-i.maximets@ovn.org Signed-off-by: Paolo Abeni <pabeni@redhat.com>
2026-07-27 14:10:21 +02:00
net: openvswitch: fix potential UAF on meter attach failure While attaching a newly created meter attach_meter() function makes the new meter visible to other CPUs but can still fail afterwards. On failure, it detaches the meter back and returns an error. However, this is an unexpected behavior for the ovs_meter_cmd_set() that uses a plain kfree(meter) on attach failure without waiting for RCU readers to stop using it, assuming it was never visible. This is never a problem for ovs-vswitchd as it always creates meters before creating any flows that use them. But the UAF can be triggered with a custom application using uAPI: BUG: KASAN: slab-use-after-free in ovs_meter_execute (net/openvswitch/meter.c:653) Read of size 8 at addr ffff88810d152650 by task meter/2508 Call Trace: ovs_meter_execute (net/openvswitch/meter.c:653) do_execute_actions (net/openvswitch/actions.c:1407) ovs_execute_actions (net/openvswitch/actions.c:1584) ovs_packet_cmd_execute (net/openvswitch/datapath.c:703) ... netlink_sendmsg (af_netlink.c:1900) Allocated by task 2519: __kasan_kmalloc (mm/kasan/common.c:398 mm/kasan/common.c:415) ovs_meter_cmd_set (net/openvswitch/meter.c:422) ... netlink_sendmsg (af_netlink.c:1900) Freed by task 2519: kfree (mm/slub.c:2705 mm/slub.c:6405 mm/slub.c:6720) ovs_meter_cmd_set (net/openvswitch/meter.c:479) ... netlink_sendmsg (af_netlink.c:1900) Fix that by making sure attach_meter() doesn't make the meter visible until all the checks are done and the function can't fail anymore. This also makes sure the "hash" value is calculated after the potential re-sizing of the table. Reported by Trend Micro's Zero Day Initiative as ZDI-CAN-31642. Fixes: c7c4c44c9a95 ("net: openvswitch: expand the meters supported number") Cc: stable@vger.kernel.org Signed-off-by: Ilya Maximets <i.maximets@ovn.org> Reviewed-by: Eelco Chaudron <echaudro@redhat.com> Link: https://patch.msgid.link/20260727121022.198461-1-i.maximets@ovn.org Signed-off-by: Paolo Abeni <pabeni@redhat.com>
2026-07-27 14:10:21 +02:00
net: openvswitch: fix potential UAF on meter attach failure While attaching a newly created meter attach_meter() function makes the new meter visible to other CPUs but can still fail afterwards. On failure, it detaches the meter back and returns an error. However, this is an unexpected behavior for the ovs_meter_cmd_set() that uses a plain kfree(meter) on attach failure without waiting for RCU readers to stop using it, assuming it was never visible. This is never a problem for ovs-vswitchd as it always creates meters before creating any flows that use them. But the UAF can be triggered with a custom application using uAPI: BUG: KASAN: slab-use-after-free in ovs_meter_execute (net/openvswitch/meter.c:653) Read of size 8 at addr ffff88810d152650 by task meter/2508 Call Trace: ovs_meter_execute (net/openvswitch/meter.c:653) do_execute_actions (net/openvswitch/actions.c:1407) ovs_execute_actions (net/openvswitch/actions.c:1584) ovs_packet_cmd_execute (net/openvswitch/datapath.c:703) ... netlink_sendmsg (af_netlink.c:1900) Allocated by task 2519: __kasan_kmalloc (mm/kasan/common.c:398 mm/kasan/common.c:415) ovs_meter_cmd_set (net/openvswitch/meter.c:422) ... netlink_sendmsg (af_netlink.c:1900) Freed by task 2519: kfree (mm/slub.c:2705 mm/slub.c:6405 mm/slub.c:6720) ovs_meter_cmd_set (net/openvswitch/meter.c:479) ... netlink_sendmsg (af_netlink.c:1900) Fix that by making sure attach_meter() doesn't make the meter visible until all the checks are done and the function can't fail anymore. This also makes sure the "hash" value is calculated after the potential re-sizing of the table. Reported by Trend Micro's Zero Day Initiative as ZDI-CAN-31642. Fixes: c7c4c44c9a95 ("net: openvswitch: expand the meters supported number") Cc: stable@vger.kernel.org Signed-off-by: Ilya Maximets <i.maximets@ovn.org> Reviewed-by: Eelco Chaudron <echaudro@redhat.com> Link: https://patch.msgid.link/20260727121022.198461-1-i.maximets@ovn.org Signed-off-by: Paolo Abeni <pabeni@redhat.com>
2026-07-27 14:10:21 +02:00
netlink: make validation more configurable for future strictness We currently have two levels of strict validation: 1) liberal (default) - undefined (type >= max) & NLA_UNSPEC attributes accepted - attribute length >= expected accepted - garbage at end of message accepted 2) strict (opt-in) - NLA_UNSPEC attributes accepted - attribute length >= expected accepted Split out parsing strictness into four different options: * TRAILING - check that there's no trailing data after parsing attributes (in message or nested) * MAXTYPE - reject attrs > max known type * UNSPEC - reject attributes with NLA_UNSPEC policy entries * STRICT_ATTRS - strictly validate attribute size The default for future things should be *everything*. The current *_strict() is a combination of TRAILING and MAXTYPE, and is renamed to _deprecated_strict(). The current regular parsing has none of this, and is renamed to *_parse_deprecated(). Additionally it allows us to selectively set one of the new flags even on old policies. Notably, the UNSPEC flag could be useful in this case, since it can be arranged (by filling in the policy) to not be an incompatible userspace ABI change, but would then going forward prevent forgetting attribute entries. Similar can apply to the POLICY flag. We end up with the following renames: * nla_parse -> nla_parse_deprecated * nla_parse_strict -> nla_parse_deprecated_strict * nlmsg_parse -> nlmsg_parse_deprecated * nlmsg_parse_strict -> nlmsg_parse_deprecated_strict * nla_parse_nested -> nla_parse_nested_deprecated * nla_validate_nested -> nla_validate_nested_deprecated Using spatch, of course: @@ expression TB, MAX, HEAD, LEN, POL, EXT; @@ -nla_parse(TB, MAX, HEAD, LEN, POL, EXT) +nla_parse_deprecated(TB, MAX, HEAD, LEN, POL, EXT) @@ expression NLH, HDRLEN, TB, MAX, POL, EXT; @@ -nlmsg_parse(NLH, HDRLEN, TB, MAX, POL, EXT) +nlmsg_parse_deprecated(NLH, HDRLEN, TB, MAX, POL, EXT) @@ expression NLH, HDRLEN, TB, MAX, POL, EXT; @@ -nlmsg_parse_strict(NLH, HDRLEN, TB, MAX, POL, EXT) +nlmsg_parse_deprecated_strict(NLH, HDRLEN, TB, MAX, POL, EXT) @@ expression TB, MAX, NLA, POL, EXT; @@ -nla_parse_nested(TB, MAX, NLA, POL, EXT) +nla_parse_nested_deprecated(TB, MAX, NLA, POL, EXT) @@ expression START, MAX, POL, EXT; @@ -nla_validate_nested(START, MAX, POL, EXT) +nla_validate_nested_deprecated(START, MAX, POL, EXT) @@ expression NLH, HDRLEN, MAX, POL, EXT; @@ -nlmsg_validate(NLH, HDRLEN, MAX, POL, EXT) +nlmsg_validate_deprecated(NLH, HDRLEN, MAX, POL, EXT) For this patch, don't actually add the strict, non-renamed versions yet so that it breaks compile if I get it wrong. Also, while at it, make nla_validate and nla_parse go down to a common __nla_validate_parse() function to avoid code duplication. Ultimately, this allows us to have very strict validation for every new caller of nla_parse()/nlmsg_parse() etc as re-introduced in the next patch, while existing things will continue to work as is. In effect then, this adds fully strict validation for any new command. Signed-off-by: Johannes Berg <johannes.berg@intel.com> Signed-off-by: David S. Miller <davem@davemloft.net>
2019-04-26 14:07:28 +02:00
openvswitch: meter: remove rate from the bucket size calculation Implementation of meters supposed to be a classic token bucket with 2 typical parameters: rate and burst size. Burst size in this schema is the maximum number of bytes/packets that could pass without being rate limited. Recent changes to userspace datapath made meter implementation to be in line with the kernel one, and this uncovered several issues. The main problem is that maximum bucket size for unknown reason accounts not only burst size, but also the numerical value of rate. This creates a lot of confusion around behavior of meters. For example, if rate is configured as 1000 pps and burst size set to 1, this should mean that meter will tolerate bursts of 1 packet at most, i.e. not a single packet above the rate should pass the meter. However, current implementation calculates maximum bucket size as (rate + burst size), so the effective bucket size will be 1001. This means that first 1000 packets will not be rate limited and average rate might be twice as high as the configured rate. This also makes it practically impossible to configure meter that will have burst size lower than the rate, which might be a desirable configuration if the rate is high. Inability to configure low values of a burst size and overall inability for a user to predict what will be a maximum and average rate from the configured parameters of a meter without looking at the OVS and kernel code might be also classified as a security issue, because drop meters are frequently used as a way of protection from DoS attacks. This change removes rate from the calculation of a bucket size, making it in line with the classic token bucket algorithm and essentially making the rate and burst tolerance being predictable from a users' perspective. Same change proposed for the userspace implementation. Fixes: 96fbc13d7e77 ("openvswitch: Add meter infrastructure") Signed-off-by: Ilya Maximets <i.maximets@ovn.org> Signed-off-by: David S. Miller <davem@davemloft.net>
2021-04-21 15:57:47 +02:00
openvswitch: meter: remove rate from the bucket size calculation Implementation of meters supposed to be a classic token bucket with 2 typical parameters: rate and burst size. Burst size in this schema is the maximum number of bytes/packets that could pass without being rate limited. Recent changes to userspace datapath made meter implementation to be in line with the kernel one, and this uncovered several issues. The main problem is that maximum bucket size for unknown reason accounts not only burst size, but also the numerical value of rate. This creates a lot of confusion around behavior of meters. For example, if rate is configured as 1000 pps and burst size set to 1, this should mean that meter will tolerate bursts of 1 packet at most, i.e. not a single packet above the rate should pass the meter. However, current implementation calculates maximum bucket size as (rate + burst size), so the effective bucket size will be 1001. This means that first 1000 packets will not be rate limited and average rate might be twice as high as the configured rate. This also makes it practically impossible to configure meter that will have burst size lower than the rate, which might be a desirable configuration if the rate is high. Inability to configure low values of a burst size and overall inability for a user to predict what will be a maximum and average rate from the configured parameters of a meter without looking at the OVS and kernel code might be also classified as a security issue, because drop meters are frequently used as a way of protection from DoS attacks. This change removes rate from the calculation of a bucket size, making it in line with the classic token bucket algorithm and essentially making the rate and burst tolerance being predictable from a users' perspective. Same change proposed for the userspace implementation. Fixes: 96fbc13d7e77 ("openvswitch: Add meter infrastructure") Signed-off-by: Ilya Maximets <i.maximets@ovn.org> Signed-off-by: David S. Miller <davem@davemloft.net>
2021-04-21 15:57:47 +02:00