From f486e8405a13667e63765d804dd0ef96f38228c8 Mon Sep 17 00:00:00 2001 From: Ben Pfaff Date: Mon, 19 Mar 2012 13:47:50 -0700 Subject: [PATCH] netdev-linux: Fix use-after-free when netdev_dump_queues() deletes queues. iface_configure_qos() passes a callback to netdev_dump_queues() that can delete queues. The netdev-linux implementation of this function was unprepared for the callback to delete queues, so this could cause a use-after-free. This fixes the problem in netdev_linux_dump_queues() and documents that netdev_dump_queues() implementations must support deletions in the callback. Found by valgrind: ==1593== Invalid read of size 8 ==1593== at 0x4A8C43: netdev_linux_dump_queues (hmap.h:326) ==1593== by 0x4305F7: bridge_reconfigure (bridge.c:3084) ==1593== by 0x431384: bridge_run (bridge.c:1892) ==1593== by 0x432749: main (ovs-vswitchd.c:96) ==1593== Address 0x632e078 is 8 bytes inside a block of size 32 free'd ==1593== at 0x4C240FD: free (vg_replace_malloc.c:366) ==1593== by 0x4A4D74: hfsc_class_delete (netdev-linux.c:3250) ==1593== by 0x42AA59: iface_delete_queues (bridge.c:3055) ==1593== by 0x4A8C8C: netdev_linux_dump_queues (netdev-linux.c:1881) ==1593== by 0x4305F7: bridge_reconfigure (bridge.c:3084) ==1593== by 0x431384: bridge_run (bridge.c:1892) Bug #10164. Reported-by: Ram Jothikumar Signed-off-by: Ben Pfaff --- lib/netdev-linux.c | 5 +++-- lib/netdev-provider.h | 9 +++++++-- lib/netdev.c | 6 +++++- 3 files changed, 15 insertions(+), 5 deletions(-) diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index 3dd5c8af0..be70a916e 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -2002,7 +2002,7 @@ netdev_linux_dump_queues(const struct netdev *netdev, { struct netdev_dev_linux *netdev_dev = netdev_dev_linux_cast(netdev_get_dev(netdev)); - struct tc_queue *queue; + struct tc_queue *queue, *next_queue; struct shash details; int last_error; int error; @@ -2016,7 +2016,8 @@ netdev_linux_dump_queues(const struct netdev *netdev, last_error = 0; shash_init(&details); - HMAP_FOR_EACH (queue, hmap_node, &netdev_dev->tc->queues) { + HMAP_FOR_EACH_SAFE (queue, next_queue, hmap_node, + &netdev_dev->tc->queues) { shash_clear(&details); error = netdev_dev->tc->ops->class_get(netdev, queue, &details); diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h index 2ef75b30c..dea171db2 100644 --- a/lib/netdev-provider.h +++ b/lib/netdev-provider.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2009, 2010, 2011 Nicira Networks. + * Copyright (c) 2009, 2010, 2011, 2012 Nicira Networks. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -464,7 +464,12 @@ struct netdev_class { * of iteration is unspecified, but (when successful) each queue is visited * exactly once. * - * 'cb' will not modify or free the 'details' argument passed in. */ + * 'cb' will not modify or free the 'details' argument passed in. It may + * delete or modify the queue passed in as its 'queue_id' argument. It may + * modify but will not delete any other queue within 'netdev'. If 'cb' + * adds new queues, then ->dump_queues is allowed to visit some queues + * twice or not at all. + */ int (*dump_queues)(const struct netdev *netdev, void (*cb)(unsigned int queue_id, const struct shash *details, diff --git a/lib/netdev.c b/lib/netdev.c index 5aa30a7a2..305ad13f6 100644 --- a/lib/netdev.c +++ b/lib/netdev.c @@ -1235,7 +1235,11 @@ netdev_get_queue_stats(const struct netdev *netdev, unsigned int queue_id, * Calling this function may be more efficient than calling netdev_get_queue() * for every queue. * - * 'cb' must not modify or free the 'details' argument passed in. + * 'cb' must not modify or free the 'details' argument passed in. It may + * delete or modify the queue passed in as its 'queue_id' argument. It may + * modify but must not delete any other queue within 'netdev'. 'cb' should not + * add new queues because this may cause some queues to be visited twice or not + * at all. * * Returns 0 if successful, otherwise a positive errno value. On error, some * configured queues may not have been included in the iteration. */ -- 2.43.0