USB: usbmon: end ugly tricks with DMA peeking

This patch fixes crashes when usbmon attempts to access GART aperture. The old code attempted to take a bus address and convert it into a virtual address, which clearly was impossible on systems with actual IOMMUs. Let us not persist in this foolishness, and use transfer_buffer in all cases instead. I think downsides are negligible. The ones I see are: - A driver may pass an address of one buffer down as transfer_buffer, and entirely different entity mapped for DMA, resulting in misleading output of usbmon. Note, however, that PIO based controllers would do transfer the same data that usbmon sees here. - Out of tree drivers may crash usbmon if they store garbage in transfer_buffer. I inspected the in-tree drivers, and clarified the documentation in comments. - Drivers that use get_user_pages will not be possible to monitor. I only found one driver with this problem (drivers/staging/rspiusb). - Same happens with with usb_storage transferring from highmem, but it works fine on 64-bit systems, so I think it's not a concern. At least we don't crash anymore. Why didn't we do this in 2.6.10? That's because back in those days it was popular not to fill in transfer_buffer, so almost all traffic would be invisible (e.g. all of HID was like that). But now, the tree is almost 100% PIO friendly, so we can do the right thing at last. Signed-off-by: Pete Zaitcev <zaitcev@redhat.com> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>

USB: usbmon: end ugly tricks with DMA peeking
This patch fixes crashes when usbmon attempts to access GART aperture. The old code attempted to take a bus address and convert it into a virtual address, which clearly was impossible on systems with actual IOMMUs. Let us not persist in this foolishness, and use transfer_buffer in all cases instead. I think downsides are negligible. The ones I see are: - A driver may pass an address of one buffer down as transfer_buffer, and entirely different entity mapped for DMA, resulting in misleading output of usbmon. Note, however, that PIO based controllers would do transfer the same data that usbmon sees here. - Out of tree drivers may crash usbmon if they store garbage in transfer_buffer. I inspected the in-tree drivers, and clarified the documentation in comments. - Drivers that use get_user_pages will not be possible to monitor. I only found one driver with this problem (drivers/staging/rspiusb). - Same happens with with usb_storage transferring from highmem, but it works fine on 64-bit systems, so I think it's not a concern. At least we don't crash anymore. Why didn't we do this in 2.6.10? That's because back in those days it was popular not to fill in transfer_buffer, so almost all traffic would be invisible (e.g. all of HID was like that). But now, the tree is almost 100% PIO friendly, so we can do the right thing at last. Signed-off-by: Pete Zaitcev <zaitcev@redhat.com> Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
Pete Zaitcev · Greg Kroah-Hartman
1 parent f4e2332cfc
Showing 7 changed files with 15 additions and 142 deletions Side-by-side Diff
drivers/usb/mon/Makefile
drivers/usb/mon/mon_bin.c
drivers/usb/mon/mon_dma.c
drivers/usb/mon/mon_main.c
drivers/usb/mon/mon_text.c
drivers/usb/mon/usb_mon.h
include/linux/usb.h
@@ -2,7 +2,7 @@
 # Makefile for USB monitor
 #
  
-usbmon-objs	:= mon_main.o mon_stat.o mon_text.o mon_bin.o mon_dma.o
+usbmon-objs	:= mon_main.o mon_stat.o mon_text.o mon_bin.o
  
 obj-$(CONFIG_USB_MON)	+= usbmon.o
@@ -220,9 +220,8 @@
  
 /*
  * This is a "chunked memcpy". It does not manipulate any counters.
- * But it returns the new offset for repeated application.
  */
-unsigned int mon_copy_to_buff(const struct mon_reader_bin *this,
+static void mon_copy_to_buff(const struct mon_reader_bin *this,
     unsigned int off, const unsigned char *from, unsigned int length)
 {
 	unsigned int step_len;
@@ -247,7 +246,6 @@
 		from += step_len;
 		length -= step_len;
 	}
-	return off;
 }
  
 /*
  
@@ -400,15 +398,8 @@
     unsigned int offset, struct urb *urb, unsigned int length)
 {
  
-	if (urb->dev->bus->uses_dma &&
-	    (urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP)) {
-		mon_dmapeek_vec(rp, offset, urb->transfer_dma, length);
-		return 0;
-	}
-
 	if (urb->transfer_buffer == NULL)
 		return 'Z';
-
 	mon_copy_to_buff(rp, offset, urb->transfer_buffer, length);
 	return 0;
 }
@@ -635,7 +626,6 @@
 	spin_lock_init(&rp->b_lock);
 	init_waitqueue_head(&rp->b_wait);
 	mutex_init(&rp->fetch_lock);
-
 	rp->b_size = BUFF_DFL;
  
 	size = sizeof(struct mon_pgmap) * (rp->b_size/CHUNK_SIZE);
-/*
- * The USB Monitor, inspired by Dave Harding's USBMon.
- *
- * mon_dma.c: Library which snoops on DMA areas.
- *
- * Copyright (C) 2005 Pete Zaitcev (zaitcev@redhat.com)
- */
-#include <linux/kernel.h>
-#include <linux/list.h>
-#include <linux/highmem.h>
-#include <asm/page.h>
-
-#include <linux/usb.h>	/* Only needed for declarations in usb_mon.h */
-#include "usb_mon.h"
-
-/*
- * PC-compatibles, are, fortunately, sufficiently cache-coherent for this.
- */
-#if defined(__i386__) || defined(__x86_64__) /* CONFIG_ARCH_I386 doesn't exit */
-#define MON_HAS_UNMAP 1
-
-#define phys_to_page(phys)	pfn_to_page((phys) >> PAGE_SHIFT)
-
-char mon_dmapeek(unsigned char *dst, dma_addr_t dma_addr, int len)
-{
-	struct page *pg;
-	unsigned long flags;
-	unsigned char *map;
-	unsigned char *ptr;
-
-	/*
-	 * On i386, a DMA handle is the "physical" address of a page.
-	 * In other words, the bus address is equal to physical address.
-	 * There is no IOMMU.
-	 */
-	pg = phys_to_page(dma_addr);
-
-	/*
-	 * We are called from hardware IRQs in case of callbacks.
-	 * But we can be called from softirq or process context in case
-	 * of submissions. In such case, we need to protect KM_IRQ0.
-	 */
-	local_irq_save(flags);
-	map = kmap_atomic(pg, KM_IRQ0);
-	ptr = map + (dma_addr & (PAGE_SIZE-1));
-	memcpy(dst, ptr, len);
-	kunmap_atomic(map, KM_IRQ0);
-	local_irq_restore(flags);
-	return 0;
-}
-
-void mon_dmapeek_vec(const struct mon_reader_bin *rp,
-    unsigned int offset, dma_addr_t dma_addr, unsigned int length)
-{
-	unsigned long flags;
-	unsigned int step_len;
-	struct page *pg;
-	unsigned char *map;
-	unsigned long page_off, page_len;
-
-	local_irq_save(flags);
-	while (length) {
-		/* compute number of bytes we are going to copy in this page */
-		step_len = length;
-		page_off = dma_addr & (PAGE_SIZE-1);
-		page_len = PAGE_SIZE - page_off;
-		if (page_len < step_len)
-			step_len = page_len;
-
-		/* copy data and advance pointers */
-		pg = phys_to_page(dma_addr);
-		map = kmap_atomic(pg, KM_IRQ0);
-		offset = mon_copy_to_buff(rp, offset, map + page_off, step_len);
-		kunmap_atomic(map, KM_IRQ0);
-		dma_addr += step_len;
-		length -= step_len;
-	}
-	local_irq_restore(flags);
-}
-
-#endif /* __i386__ */
-
-#ifndef MON_HAS_UNMAP
-char mon_dmapeek(unsigned char *dst, dma_addr_t dma_addr, int len)
-{
-	return 'D';
-}
-
-void mon_dmapeek_vec(const struct mon_reader_bin *rp,
-    unsigned int offset, dma_addr_t dma_addr, unsigned int length)
-{
-	;
-}
-
-#endif /* MON_HAS_UNMAP */
@@ -361,7 +361,6 @@
 	}
 	// MOD_INC_USE_COUNT(which_module?);
  
-
 	mutex_lock(&usb_bus_list_lock);
 	list_for_each_entry (ubus, &usb_bus_list, bus_list) {
 		mon_bus_init(ubus);
@@ -150,20 +150,6 @@
 			return '>';
 	}
  
-	/*
-	 * The check to see if it's safe to poke at data has an enormous
-	 * number of corner cases, but it seems that the following is
-	 * more or less safe.
-	 *
-	 * We do not even try to look at transfer_buffer, because it can
-	 * contain non-NULL garbage in case the upper level promised to
-	 * set DMA for the HCD.
-	 */
-	if (urb->dev->bus->uses_dma &&
-	    (urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP)) {
-		return mon_dmapeek(ep->data, urb->transfer_dma, len);
-	}
-
 	if (urb->transfer_buffer == NULL)
 		return 'Z';	/* '0' would be not as pretty. */
  
@@ -65,20 +65,6 @@
 void mon_bin_exit(void);
  
 /*
- * DMA interface.
- *
- * XXX The vectored side needs a serious re-thinking. Abstracting vectors,
- * like in Paolo's original patch, produces a double pkmap. We need an idea.
-*/
-extern char mon_dmapeek(unsigned char *dst, dma_addr_t dma_addr, int len);
-
-struct mon_reader_bin;
-extern void mon_dmapeek_vec(const struct mon_reader_bin *rp,
-    unsigned int offset, dma_addr_t dma_addr, unsigned int len);
-extern unsigned int mon_copy_to_buff(const struct mon_reader_bin *rp,
-    unsigned int offset, const unsigned char *from, unsigned int len);
-
-/*
  */
 extern struct mutex mon_lock;
  
@@ -1036,9 +1036,10 @@
  * @transfer_flags: A variety of flags may be used to affect how URB
  *	submission, unlinking, or operation are handled.  Different
  *	kinds of URB can use different flags.
- * @transfer_buffer:  This identifies the buffer to (or from) which
- * 	the I/O request will be performed (unless URB_NO_TRANSFER_DMA_MAP
- *	is set).  This buffer must be suitable for DMA; allocate it with
+ * @transfer_buffer:  This identifies the buffer to (or from) which the I/O
+ *	request will be performed unless URB_NO_TRANSFER_DMA_MAP is set
+ *	(however, do not leave garbage in transfer_buffer even then).
+ *	This buffer must be suitable for DMA; allocate it with
  *	kmalloc() or equivalent.  For transfers to "in" endpoints, contents
  *	of this buffer will be modified.  This buffer is used for the data
  *	stage of control transfers.
@@ -1104,9 +1105,15 @@
  * allocate a DMA buffer with usb_buffer_alloc() or call usb_buffer_map().
  * When these transfer flags are provided, host controller drivers will
  * attempt to use the dma addresses found in the transfer_dma and/or
- * setup_dma fields rather than determining a dma address themselves.  (Note
- * that transfer_buffer and setup_packet must still be set because not all
- * host controllers use DMA, nor do virtual root hubs).
+ * setup_dma fields rather than determining a dma address themselves.
+ *
+ * Note that transfer_buffer must still be set if the controller
+ * does not support DMA (as indicated by bus.uses_dma) and when talking
+ * to root hub. If you have to trasfer between highmem zone and the device
+ * on such controller, create a bounce buffer or bail out with an error.
+ * If transfer_buffer cannot be set (is in highmem) and the controller is DMA
+ * capable, assign NULL to it, so that usbmon knows not to use the value.
+ * The setup_packet must always be set, so it cannot be located in highmem.
  *
  * Initialization:
  *
...	...	@@ -2,7 +2,7 @@
2	2	# Makefile for USB monitor
3	3	#
4	4
5		-usbmon-objs := mon_main.o mon_stat.o mon_text.o mon_bin.o mon_dma.o
	5	+usbmon-objs := mon_main.o mon_stat.o mon_text.o mon_bin.o
6	6
7	7	obj-$(CONFIG_USB_MON) += usbmon.o
...	...	@@ -220,9 +220,8 @@
220	220
221	221	/*
222	222	* This is a "chunked memcpy". It does not manipulate any counters.
223		- * But it returns the new offset for repeated application.
224	223	*/
225		-unsigned int mon_copy_to_buff(const struct mon_reader_bin *this,
	224	+static void mon_copy_to_buff(const struct mon_reader_bin *this,
226	225	unsigned int off, const unsigned char *from, unsigned int length)
227	226	{
228	227	unsigned int step_len;
...	...	@@ -247,7 +246,6 @@
247	246	from += step_len;
248	247	length -= step_len;
249	248	}
250		- return off;
251	249	}
252	250
253	251	/*
254	252
...	...	@@ -400,15 +398,8 @@
400	398	unsigned int offset, struct urb *urb, unsigned int length)
401	399	{
402	400
403		- if (urb->dev->bus->uses_dma &&
404		- (urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP)) {
405		- mon_dmapeek_vec(rp, offset, urb->transfer_dma, length);
406		- return 0;
407		- }
408		-
409	401	if (urb->transfer_buffer == NULL)
410	402	return 'Z';
411		-
412	403	mon_copy_to_buff(rp, offset, urb->transfer_buffer, length);
413	404	return 0;
414	405	}
...	...	@@ -635,7 +626,6 @@
635	626	spin_lock_init(&rp->b_lock);
636	627	init_waitqueue_head(&rp->b_wait);
637	628	mutex_init(&rp->fetch_lock);
638		-
639	629	rp->b_size = BUFF_DFL;
640	630
641	631	size = sizeof(struct mon_pgmap) * (rp->b_size/CHUNK_SIZE);
1		-/*
2		- * The USB Monitor, inspired by Dave Harding's USBMon.
3		- *
4		- * mon_dma.c: Library which snoops on DMA areas.
5		- *
6		- * Copyright (C) 2005 Pete Zaitcev (zaitcev@redhat.com)
7		- */
8		-#include <linux/kernel.h>
9		-#include <linux/list.h>
10		-#include <linux/highmem.h>
11		-#include <asm/page.h>
12		-
13		-#include <linux/usb.h> /* Only needed for declarations in usb_mon.h */
14		-#include "usb_mon.h"
15		-
16		-/*
17		- * PC-compatibles, are, fortunately, sufficiently cache-coherent for this.
18		- */
19		-#if defined(__i386__) \|\| defined(__x86_64__) /* CONFIG_ARCH_I386 doesn't exit */
20		-#define MON_HAS_UNMAP 1
21		-
22		-#define phys_to_page(phys) pfn_to_page((phys) >> PAGE_SHIFT)
23		-
24		-char mon_dmapeek(unsigned char *dst, dma_addr_t dma_addr, int len)
25		-{
26		- struct page *pg;
27		- unsigned long flags;
28		- unsigned char *map;
29		- unsigned char *ptr;
30		-
31		- /*
32		- * On i386, a DMA handle is the "physical" address of a page.
33		- * In other words, the bus address is equal to physical address.
34		- * There is no IOMMU.
35		- */
36		- pg = phys_to_page(dma_addr);
37		-
38		- /*
39		- * We are called from hardware IRQs in case of callbacks.
40		- * But we can be called from softirq or process context in case
41		- * of submissions. In such case, we need to protect KM_IRQ0.
42		- */
43		- local_irq_save(flags);
44		- map = kmap_atomic(pg, KM_IRQ0);
45		- ptr = map + (dma_addr & (PAGE_SIZE-1));
46		- memcpy(dst, ptr, len);
47		- kunmap_atomic(map, KM_IRQ0);
48		- local_irq_restore(flags);
49		- return 0;
50		-}
51		-
52		-void mon_dmapeek_vec(const struct mon_reader_bin *rp,
53		- unsigned int offset, dma_addr_t dma_addr, unsigned int length)
54		-{
55		- unsigned long flags;
56		- unsigned int step_len;
57		- struct page *pg;
58		- unsigned char *map;
59		- unsigned long page_off, page_len;
60		-
61		- local_irq_save(flags);
62		- while (length) {
63		- /* compute number of bytes we are going to copy in this page */
64		- step_len = length;
65		- page_off = dma_addr & (PAGE_SIZE-1);
66		- page_len = PAGE_SIZE - page_off;
67		- if (page_len < step_len)
68		- step_len = page_len;
69		-
70		- /* copy data and advance pointers */
71		- pg = phys_to_page(dma_addr);
72		- map = kmap_atomic(pg, KM_IRQ0);
73		- offset = mon_copy_to_buff(rp, offset, map + page_off, step_len);
74		- kunmap_atomic(map, KM_IRQ0);
75		- dma_addr += step_len;
76		- length -= step_len;
77		- }
78		- local_irq_restore(flags);
79		-}
80		-
81		-#endif /* __i386__ */
82		-
83		-#ifndef MON_HAS_UNMAP
84		-char mon_dmapeek(unsigned char *dst, dma_addr_t dma_addr, int len)
85		-{
86		- return 'D';
87		-}
88		-
89		-void mon_dmapeek_vec(const struct mon_reader_bin *rp,
90		- unsigned int offset, dma_addr_t dma_addr, unsigned int length)
91		-{
92		- ;
93		-}
94		-
95		-#endif /* MON_HAS_UNMAP */
...	...	@@ -361,7 +361,6 @@
361	361	}
362	362	// MOD_INC_USE_COUNT(which_module?);
363	363
364		-
365	364	mutex_lock(&usb_bus_list_lock);
366	365	list_for_each_entry (ubus, &usb_bus_list, bus_list) {
367	366	mon_bus_init(ubus);
...	...	@@ -150,20 +150,6 @@
150	150	return '>';
151	151	}
152	152
153		- /*
154		- * The check to see if it's safe to poke at data has an enormous
155		- * number of corner cases, but it seems that the following is
156		- * more or less safe.
157		- *
158		- * We do not even try to look at transfer_buffer, because it can
159		- * contain non-NULL garbage in case the upper level promised to
160		- * set DMA for the HCD.
161		- */
162		- if (urb->dev->bus->uses_dma &&
163		- (urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP)) {
164		- return mon_dmapeek(ep->data, urb->transfer_dma, len);
165		- }
166		-
167	153	if (urb->transfer_buffer == NULL)
168	154	return 'Z'; /* '0' would be not as pretty. */
169	155
...	...	@@ -65,20 +65,6 @@
65	65	void mon_bin_exit(void);
66	66
67	67	/*
68		- * DMA interface.
69		- *
70		- * XXX The vectored side needs a serious re-thinking. Abstracting vectors,
71		- * like in Paolo's original patch, produces a double pkmap. We need an idea.
72		-*/
73		-extern char mon_dmapeek(unsigned char *dst, dma_addr_t dma_addr, int len);
74		-
75		-struct mon_reader_bin;
76		-extern void mon_dmapeek_vec(const struct mon_reader_bin *rp,
77		- unsigned int offset, dma_addr_t dma_addr, unsigned int len);
78		-extern unsigned int mon_copy_to_buff(const struct mon_reader_bin *rp,
79		- unsigned int offset, const unsigned char *from, unsigned int len);
80		-
81		-/*
82	68	*/
83	69	extern struct mutex mon_lock;
84	70
...	...	@@ -1036,9 +1036,10 @@
1036	1036	* @transfer_flags: A variety of flags may be used to affect how URB
1037	1037	* submission, unlinking, or operation are handled. Different
1038	1038	* kinds of URB can use different flags.
1039		- * @transfer_buffer: This identifies the buffer to (or from) which
1040		- * the I/O request will be performed (unless URB_NO_TRANSFER_DMA_MAP
1041		- * is set). This buffer must be suitable for DMA; allocate it with
	1039	+ * @transfer_buffer: This identifies the buffer to (or from) which the I/O
	1040	+ * request will be performed unless URB_NO_TRANSFER_DMA_MAP is set
	1041	+ * (however, do not leave garbage in transfer_buffer even then).
	1042	+ * This buffer must be suitable for DMA; allocate it with
1042	1043	* kmalloc() or equivalent. For transfers to "in" endpoints, contents
1043	1044	* of this buffer will be modified. This buffer is used for the data
1044	1045	* stage of control transfers.
...	...	@@ -1104,9 +1105,15 @@
1104	1105	* allocate a DMA buffer with usb_buffer_alloc() or call usb_buffer_map().
1105	1106	* When these transfer flags are provided, host controller drivers will
1106	1107	* attempt to use the dma addresses found in the transfer_dma and/or
1107		- * setup_dma fields rather than determining a dma address themselves. (Note
1108		- * that transfer_buffer and setup_packet must still be set because not all
1109		- * host controllers use DMA, nor do virtual root hubs).
	1108	+ * setup_dma fields rather than determining a dma address themselves.
	1109	+ *
	1110	+ * Note that transfer_buffer must still be set if the controller
	1111	+ * does not support DMA (as indicated by bus.uses_dma) and when talking
	1112	+ * to root hub. If you have to trasfer between highmem zone and the device
	1113	+ * on such controller, create a bounce buffer or bail out with an error.
	1114	+ * If transfer_buffer cannot be set (is in highmem) and the controller is DMA
	1115	+ * capable, assign NULL to it, so that usbmon knows not to use the value.
	1116	+ * The setup_packet must always be set, so it cannot be located in highmem.
1110	1117	*
1111	1118	* Initialization:
1112	1119	*