Re: [PATCH] Fix for kernel crash with udav(4) device

2011-03-21 Thread Matthias Kilian 
On Sun, Mar 20, 2011 at 04:07:33PM -0400, Loganaden Velvindron wrote:
 With input from mk, we improved the diff.
 
 Testing is very much appreciated.
[...]

I can't comment on the code (it isn't my area, but, worse, i'm still
too short of time), but at least a make build over nfs now finished
without any problems.

Ciao,
Kili

 Index: src/sys/dev/usb/if_udav.c
 ===
 RCS file: /cvs/src/sys/dev/usb/if_udav.c,v
 retrieving revision 1.54
 diff -u -p -r1.54 if_udav.c
 --- src/sys/dev/usb/if_udav.c 20 Mar 2011 17:58:26 -  1.54
 +++ src/sys/dev/usb/if_udav.c 20 Mar 2011 19:58:51 -
 @@ -1128,18 +1128,25 @@ udav_rxeof(usbd_xfer_handle xfer, usbd_p
  
   usbd_get_xfer_status(xfer, NULL, NULL, total_len, NULL);
  
 + if (total_len  UDAV_RX_HDRLEN) {
 + ifp-if_ierrors++;
 + goto done;
 + }
 + 
   h = (struct udav_rx_hdr *)c-udav_buf;
   total_len = UGETW(h-length) - ETHER_CRC_LEN;
   
 - DPRINTF((%s: RX Status: 0x%02x\n, h-pktstat));
 + DPRINTF((%s: RX Status: 0x%02x\n, sc-sc_dev.dv_xname, h-pktstat));
  
   if (h-pktstat  UDAV_RSR_LCS) {
   ifp-if_collisions++;
   goto done;
   }
  
 + /* RX status may still be correct but total_len is bogus */
   if (total_len  sizeof(struct ether_header) ||
 - h-pktstat  UDAV_RSR_ERR) {
 + h-pktstat  UDAV_RSR_ERR ||
 + total_len  UDAV_BUFSZ ) {
   ifp-if_ierrors++;
   goto done;
   }
 Index: src/sys/dev/usb/if_udavreg.h
 ===
 RCS file: /cvs/src/sys/dev/usb/if_udavreg.h,v
 retrieving revision 1.11
 diff -u -p -r1.11 if_udavreg.h
 --- src/sys/dev/usb/if_udavreg.h  6 Dec 2010 04:41:39 -   1.11
 +++ src/sys/dev/usb/if_udavreg.h  20 Mar 2011 19:58:51 -
 @@ -200,6 +200,6 @@ struct udav_rx_hdr {
  #define UDAV_RX_HDRLEN   sizeof(struct udav_rx_hdr)
  
  /* Packet length */
 -#define  UDAV_MAX_MTU1536 /* XXX: max frame size is unknown 
 */
 +#define  UDAV_MAX_MTU1522 /* According to datasheet  */
  #define  UDAV_MIN_FRAME_LEN  60
  #define  UDAV_BUFSZ  UDAV_MAX_MTU + UDAV_RX_HDRLEN

-- 
Password:
You speak an infinite deal of nothing
-- sudo(8), OpenBSD

Re: [PATCH] Fix for kernel crash with udav(4) device

2011-03-20 Thread Loganaden Velvindron 
With input from mk, we improved the diff.

Testing is very much appreciated.

mk@ suggests checking total_len right after xfer_status to make
sure the header is safe to use.

There's a mistake in the DPRINTF() fo RX status, one parameter is missing.
This caused another crash when debug mode is enabled.

it discards packets that are more than UDAV_BUFSZ even if the status
is correct (This is from mk@).

The datasheet mentions that a normal packet is 1522 bytes.

It works with no regression on my udav(4).

Index: src/sys/dev/usb/if_udav.c
===
RCS file: /cvs/src/sys/dev/usb/if_udav.c,v
retrieving revision 1.54
diff -u -p -r1.54 if_udav.c
--- src/sys/dev/usb/if_udav.c   20 Mar 2011 17:58:26 -  1.54
+++ src/sys/dev/usb/if_udav.c   20 Mar 2011 19:58:51 -
@@ -1128,18 +1128,25 @@ udav_rxeof(usbd_xfer_handle xfer, usbd_p
 
usbd_get_xfer_status(xfer, NULL, NULL, total_len, NULL);
 
+   if (total_len  UDAV_RX_HDRLEN) {
+   ifp-if_ierrors++;
+   goto done;
+   }
+   
h = (struct udav_rx_hdr *)c-udav_buf;
total_len = UGETW(h-length) - ETHER_CRC_LEN;

-   DPRINTF((%s: RX Status: 0x%02x\n, h-pktstat));
+   DPRINTF((%s: RX Status: 0x%02x\n, sc-sc_dev.dv_xname, h-pktstat));
 
if (h-pktstat  UDAV_RSR_LCS) {
ifp-if_collisions++;
goto done;
}
 
+   /* RX status may still be correct but total_len is bogus */
if (total_len  sizeof(struct ether_header) ||
-   h-pktstat  UDAV_RSR_ERR) {
+   h-pktstat  UDAV_RSR_ERR ||
+   total_len  UDAV_BUFSZ ) {
ifp-if_ierrors++;
goto done;
}
Index: src/sys/dev/usb/if_udavreg.h
===
RCS file: /cvs/src/sys/dev/usb/if_udavreg.h,v
retrieving revision 1.11
diff -u -p -r1.11 if_udavreg.h
--- src/sys/dev/usb/if_udavreg.h6 Dec 2010 04:41:39 -   1.11
+++ src/sys/dev/usb/if_udavreg.h20 Mar 2011 19:58:51 -
@@ -200,6 +200,6 @@ struct udav_rx_hdr {
 #define UDAV_RX_HDRLEN sizeof(struct udav_rx_hdr)
 
 /* Packet length */
-#defineUDAV_MAX_MTU1536 /* XXX: max frame size is unknown 
*/
+#defineUDAV_MAX_MTU1522 /* According to datasheet  */
 #defineUDAV_MIN_FRAME_LEN  60
 #defineUDAV_BUFSZ  UDAV_MAX_MTU + UDAV_RX_HDRLEN

Re: [PATCH] Fix for kernel crash with udav(4) device

2011-03-17 Thread Loganaden Velvindron 
Ok, I tested it with an NFS server, and also tested it with 
SFTP. The original diff works fine with the original diff.

udav(4) uses 1514 bytes for largest values of total_len.

//Logan
C-x-C-c

[PATCH] Fix for kernel crash with udav(4) device

2011-03-14 Thread Loganaden Velvindron 
This device:
udav0 at uhub5 port 2 ShanTou DM9601 rev 1.10/1.01 addr 2
udav0: address 00:60:6e:00:6e:20
amphy0 at udav0 phy 0: DM9601 10/100 PHY, rev. 0

causes a kernel crash with the following messages:
memcpy() at memcpy+0x16
usb_transfer_complete() at usb_transfer_complete+0x256
uhci_softintr() at uchi_softintr+0x40
softintr_dispatch() at softintr_dispatch+0x5d

end trace frame:0x0,count:-5

Using printf() with total_len shows that at certain times,
it is 54768, where it should be less than the maximum frame size.

Experimentally, the maximum value of total_len is 1514, but
in if_udavreg.h, it is 1536.

Index: src/sys/dev/usb/if_udav.c
===
RCS file: /cvs/src/sys/dev/usb/if_udav.c,v
retrieving revision 1.51
diff -u -p -r1.51 if_udav.c
--- src/sys/dev/usb/if_udav.c   25 Jan 2011 20:03:35 -  1.51
+++ src/sys/dev/usb/if_udav.c   14 Mar 2011 12:17:40 -
@@ -1139,6 +1139,7 @@ udav_rxeof(usbd_xfer_handle xfer, usbd_p
}
 
if (total_len  sizeof(struct ether_header) ||
+   total_len  UDAV_MAX_MTU ||
h-pktstat  UDAV_RSR_ERR) {
ifp-if_ierrors++;
goto done;