netdev - multiqueue, skb_get_queue_mapping() and netdev_get_tx

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 14 Jul 2010 17:13:53 -0600
From:	Eldon Koyle <esk-netdev@....cs.usu.edu>
To:	netdev@...r.kernel.org
Subject: multiqueue, skb_get_queue_mapping() and netdev_get_tx_queue()

It looks like there is a potential for an out of bounds index anywhere
skb_get_queue_mapping(skb) (which just returns skb->queue_mapping) is
used to get an index for netdev_get_tx_queue() (and probably other
places) on a device with multiple rx/tx queues.

As I understand it, skb->queue_mapping should contain rx_queue + 1,
which can be out of range for netdev_get_tx_queue (which expects a
0-based index).

Am I misunderstanding something, or should all of these occurrences be
replaced with something more like the following?

static inline u16 skb_get_queue_index(const struct sk_buff *skb)
{
        return skb->queue_mapping ? skb->queue_mapping - 1 : 0;
}

Here is how it is commonly used (which looks incorrect to me):
In net/8021q/vlan_dev.c:
	static netdev_tx_t vlan_dev_hard_start_xmit(struct sk_buff *skb,
						    struct net_device *dev)
	{
		int i = skb_get_queue_mapping(skb);
		struct netdev_queue *txq = netdev_get_tx_queue(dev, i);
	...


And here is some other possibly pertinent code:
In include/linux/netdevice.h:
	static inline
	struct netdev_queue *netdev_get_tx_queue(const struct net_device *dev,
						 unsigned int index)
	{
		return &dev->_tx[index];
	}

In net/core/dev.c:
	struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
			void (*setup)(struct net_device *), unsigned int queue_count)
	...
		tx = kcalloc(queue_count, sizeof(struct netdev_queue), GFP_KERNEL);
	...
		dev->_tx = tx;
	...

In include/linux/skbuff.h:
	static inline u16 skb_get_queue_mapping(const struct sk_buff *skb)
	{
		return skb->queue_mapping;
	}
	...
	static inline void skb_record_rx_queue(struct sk_buff *skb, u16 rx_queue)
	{
		skb->queue_mapping = rx_queue + 1;
	}

	static inline u16 skb_get_rx_queue(const struct sk_buff *skb)
	{
		return skb->queue_mapping - 1;
	}


-- 
Eldon Koyle
-- 
Politicians are the same all over.  They promise to build a bridge even
where there is no river.
		-- Nikita Khrushchev
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html