On 7/27/26 2:10 PM, Ilya Maximets wrote:
> 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: [email protected]
> Signed-off-by: Ilya Maximets <[email protected]>
> ---
Just addressing some sashiko comments below. They are either insignificant
or for other pre-existing issues:
> net/openvswitch/meter.c | 33 +++++++++++++++++++--------------
> 1 file changed, 19 insertions(+), 14 deletions(-)
>
> diff --git a/net/openvswitch/meter.c b/net/openvswitch/meter.c
> index a02c47277337..4aaeeae3af5b 100644
> --- a/net/openvswitch/meter.c
> +++ b/net/openvswitch/meter.c
> @@ -133,18 +133,10 @@ static void dp_meter_instance_remove(struct
> dp_meter_instance *ti,
>
> static int attach_meter(struct dp_meter_table *tbl, struct dp_meter *meter)
> {
> - struct dp_meter_instance *ti = rcu_dereference_ovsl(tbl->ti);
> - u32 hash = meter_hash(ti, meter->id);
> + struct dp_meter_instance *ti;
> + u32 hash;
> int err;
>
> - /* In generally, slots selected should be empty, because
> - * OvS uses id-pool to fetch a available id.
> - */
> - if (unlikely(rcu_dereference_ovsl(ti->dp_meters[hash])))
> - return -EBUSY;
> -
> - dp_meter_instance_insert(ti, meter);
> -
> /* That function is thread-safe. */
> tbl->count++;
Sashiko complains about the comment, but it is fine. It is pointing out
that it's safe to increment the count, which is still the case.
> if (tbl->count >= tbl->max_meters_allowed) {
> @@ -152,16 +144,29 @@ static int attach_meter(struct dp_meter_table *tbl,
> struct dp_meter *meter)
> goto attach_err;
> }
>
> - if (tbl->count >= ti->n_meters &&
> - dp_meter_instance_realloc(tbl, ti->n_meters * 2)) {
> - err = -ENOMEM;
> + ti = rcu_dereference_ovsl(tbl->ti);
> + if (tbl->count >= ti->n_meters) {
> + err = dp_meter_instance_realloc(tbl, ti->n_meters * 2);
Both sashiko instances point out that realloc doesn't reshash the entries,
which is a known behavior of the meter table. It expects meters to be added
sequentially and not with the random ids, as that is how ovs-vswitchd
operates. This is not a good assumption to have and the table should be
replaced by a real hash table, but it is a separate work item.
> + if (err)
> + goto attach_err;
> +
> + ti = rcu_dereference_ovsl(tbl->ti);
> + }
> +
> + hash = meter_hash(ti, meter->id);
> +
> + /* In general, selected slots should be empty, because
> + * OvS uses id-pool to fetch available ids.
> + */
> + if (unlikely(rcu_dereference_ovsl(ti->dp_meters[hash]))) {
> + err = -EBUSY;
> goto attach_err;
> }
>
> + dp_meter_instance_insert(ti, meter);
It also complains that the caller relies on the fact that attach_meter()
doesn't make the pointer visible. However, IMO, this is the expected
behavior from this function and doesn't require extra comments. The
opposite behavior would warrant a comment.
> return 0;
>
> attach_err:
> - dp_meter_instance_remove(ti, meter);
> tbl->count--;
> return err;
> }
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
