Re: [PATCH] drivers/usb/host/uhci-* : check buffer length to avoid memory overflow

2013-01-08 Thread Alan Stern 
Gang:

I apologize for taking so long to respond to your patch.  I didn't get 
much work done during the holidays...

Overall it looks pretty good.  There are a few things that I would 
prefer to change.

On Thu, 20 Dec 2012, Chen Gang wrote:

   reason (why):
 for function uhci_sprint_schedule:
   the buffer len is MAX_OUTPUT: 64 * 1024
   the buffer may not be enough:
 may loop UHCI_NUMFRAMES times (UHCI_NUMFRAMES is 1024)
 each time of loop may get more than 64 bytes
   so need check the buffer length to avoid memory overflow
 
   goal (what):
 this patch fixes this correctness issue.
 
   design (and why):
 at first, we make enough room for buffering the exceeding contents
 judge the contents which we have written whether bigger than buffer length
 if bigger (the exceeding contents will be in the exceeding buffer)
   break current work flow, and return
 
   test:
 plan:
   let MAX_OUTPUT as various values:
 some values which are enough for use, then can get full contents.
 some values which small enough, so can truncate in various locations.
   check the result:
 cat the contents from the /sys/kernel/debug/usb/uhci/*
 use wc -c to get the count of output contents (match the MAX_OUTPUT)
 
 result:
   make the buffer size in different size: (1024 is the room for exceeding)
   63 * 1024 + 1024
 1st (debug == 3)   pass
 2nd (debug == 3)   pass
 3rd (debug == 1)   pass
 4rd (not define DEBUG) pass
   1689 + 1024 (debug == 3) pass (1689 is full contents length of my test)
   1688 + 1024 (debug == 3) pass
   1024 + 1024 (debug == 3) pass
512 + 1024 (debug == 3) pass
 30 + 1024 (debug == 3) pass
 
 left:
   not test it by calling from uhci-q.c and uhci-hcd.c
   not test the dump objects which already have rich data contents
   if testing them are necessary:
 please tell me, I will try.
 and it is better to tell me how to test them.

Can you rewrite this using ordinary English sentences?  Describe what 
you changed and why you changed it.  You don't have to include the 
details of your tests.

 --- a/drivers/usb/host/uhci-debug.c
 +++ b/drivers/usb/host/uhci-debug.c
 @@ -16,6 +16,8 @@
  
  #include uhci-hcd.h
  
 +#define LEFT_OUTPUT  (1024)

A better name would be EXTRA_SPACE.

 @@ -90,7 +90,11 @@ static int uhci_show_td(struct uhci_hcd *uhci, struct 
 uhci_td *td, char *buf,
   spid);
   out += sprintf(out, (buf=%08x)\n, hc32_to_cpu(uhci, td-buffer));
  
 +tail:
   return out - buf;
 +truncate:
 + out += sprintf(out,  (%s truncated)\n, __FUNCTION__);
 + goto tail;
  }

Here and in all the other functions, it would be better to do this:

+ done:
+   if (out - buf  len)
+   out += sprintf(out,  ...\n);
return out - buf;
 }

You don't need to write the word truncated all the time.  A simple 
... will let people know what happened.

Have all the goto statements jump to done.  The reason for doing it 
this way is...

 @@ -135,6 +142,8 @@ static int uhci_show_urbp(struct uhci_hcd *uhci, struct 
 urb_priv *urbp,
   out += sprintf(out, %*s%d: , space + 2, , i);
   out += uhci_show_td(uhci, td, out,
   len - (out - buf), 0);
 + if (out - buf  len)
 + goto truncate;

... here you can jump directly to the return statement and avoid 
printing out a duplicate ... line.

 @@ -211,9 +224,12 @@ static int uhci_show_qh(struct uhci_hcd *uhci,
   space, );
   i = nurbs = 0;
   list_for_each_entry(urbp, qh-queue, node) {
 - if (++i = 10)
 + if (++i = 10) {
   out += uhci_show_urbp(uhci, urbp, out,
   len - (out - buf), space + 2);
 + if (out - buf  len)
 + goto truncate;

Same here.

 @@ -222,24 +238,25 @@ static int uhci_show_qh(struct uhci_hcd *uhci,
   space, , nurbs);
   }
  
 + if (out - buf  len)
 + goto truncate;
 +
   if (qh-dummy_td) {
   out += sprintf(out, %*s  Dummy TD\n, space, );
   out += uhci_show_td(uhci, qh-dummy_td, out,
   len - (out - buf), 0);

And here.

 @@ -360,9 +383,13 @@ static int uhci_sprint_schedule(struct uhci_hcd *uhci, 
 char *buf, int len)
   int8, int4, int2, async, term
   };
  
 - out += uhci_show_root_hub_state(uhci, out, len - (out - buf));
 + out += uhci_show_root_hub_state(uhci, out);
 + if (out - buf  len)
 + goto truncate;

And here.

   out += sprintf(out, HC status\n);
   out += uhci_show_status(uhci, out, len -

Re: [PATCH] drivers/usb/host/uhci-* : check buffer length to avoid memory overflow

2013-01-08 Thread Chen Gang 
于 2013年01月09日 04:53, Alan Stern 写道:
 Gang:
 
 I apologize for taking so long to respond to your patch.  I didn't get 
 much work done during the holidays...
 
  I understand. it is necessary to have a rest, so can keep our
contributes (work), with efficiency and sustainable.

  :-)


 Can you rewrite this using ordinary English sentences?  Describe what 
 you changed and why you changed it.  You don't have to include the 
 details of your tests.
 
  ok, thanks.


 +#define LEFT_OUTPUT (1024)
 
 A better name would be EXTRA_SPACE.
 
  ok, thanks.


 @@ -90,7 +90,11 @@ static int uhci_show_td(struct uhci_hcd *uhci, struct 
 uhci_td *td, char *buf,
  spid);
  out += sprintf(out, (buf=%08x)\n, hc32_to_cpu(uhci, td-buffer));
  
 +tail:
  return out - buf;
 +truncate:
 +out += sprintf(out,  (%s truncated)\n, __FUNCTION__);
 +goto tail;
  }
 
 Here and in all the other functions, it would be better to do this:
 
 + done:
 + if (out - buf  len)
 + out += sprintf(out,  ...\n);
   return out - buf;
  }
  ok, thanks.

 
 You don't need to write the word truncated all the time.  A simple 
 ... will let people know what happened.
 
 Have all the goto statements jump to done.  The reason for doing it 
 this way is...
 
  ok, thanks.


 @@ -135,6 +142,8 @@ static int uhci_show_urbp(struct uhci_hcd *uhci, struct 
 urb_priv *urbp,
  out += sprintf(out, %*s%d: , space + 2, , i);
  out += uhci_show_td(uhci, td, out,
  len - (out - buf), 0);
 +if (out - buf  len)
 +goto truncate;
 
 .. here you can jump directly to the return statement and avoid 
 printing out a duplicate ... line.

  ok, thanks.

  I think your meaning is:
use goto tail; instead of goto truncate;
and the return area like this:

+ done:
+   if (out - buf  len)
+   out += sprintf(out,  ...\n);
+ tail:
return out - buf;
 }

  if I misunderstanding, please reply.
(no reply means I am not misunderstanding)


 @@ -375,14 +402,19 @@ static int uhci_sprint_schedule(struct uhci_hcd *uhci, 
 char *buf, int len)
  uhci_to_hcd(uhci)-self.bandwidth_int_reqs,
  uhci_to_hcd(uhci)-self.bandwidth_isoc_reqs);
  if (debug = 1)
 -return out - buf;
 +goto tail;
  
  out += sprintf(out, Frame List\n);
 +if (out - buf  len)
 +goto truncate;
 
 This test isn't needed because there's another test inside the loop 
 below.
 
  ok, thanks.



  I will send patch v2 within 4 days
   (execuse me, today and tommorrow, I have to do another things)

-- 
Chen Gang

Asianux Corporation
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] drivers/usb/host/uhci-* : check buffer length to avoid memory overflow

2012-12-20 Thread Chen Gang 

  reason (why):
for function uhci_sprint_schedule:
  the buffer len is MAX_OUTPUT: 64 * 1024
  the buffer may not be enough:
may loop UHCI_NUMFRAMES times (UHCI_NUMFRAMES is 1024)
each time of loop may get more than 64 bytes
  so need check the buffer length to avoid memory overflow

  goal (what):
this patch fixes this correctness issue.

  design (and why):
at first, we make enough room for buffering the exceeding contents
judge the contents which we have written whether bigger than buffer length
if bigger (the exceeding contents will be in the exceeding buffer)
  break current work flow, and return

  test:
plan:
  let MAX_OUTPUT as various values:
some values which are enough for use, then can get full contents.
some values which small enough, so can truncate in various locations.
  check the result:
cat the contents from the /sys/kernel/debug/usb/uhci/*
use wc -c to get the count of output contents (match the MAX_OUTPUT)

result:
  make the buffer size in different size: (1024 is the room for exceeding)
  63 * 1024 + 1024
1st (debug == 3)   pass
2nd (debug == 3)   pass
3rd (debug == 1)   pass
4rd (not define DEBUG) pass
  1689 + 1024 (debug == 3) pass (1689 is full contents length of my test)
  1688 + 1024 (debug == 3) pass
  1024 + 1024 (debug == 3) pass
   512 + 1024 (debug == 3) pass
30 + 1024 (debug == 3) pass

left:
  not test it by calling from uhci-q.c and uhci-hcd.c
  not test the dump objects which already have rich data contents
  if testing them are necessary:
please tell me, I will try.
and it is better to tell me how to test them.

Signed-off-by: Chen Gang gang.c...@asianux.com
---
 drivers/usb/host/uhci-debug.c |  140 +
 drivers/usb/host/uhci-hcd.c   |4 +-
 drivers/usb/host/uhci-q.c |2 +-
 3 files changed, 102 insertions(+), 44 deletions(-)

diff --git a/drivers/usb/host/uhci-debug.c b/drivers/usb/host/uhci-debug.c
index fc0b0da..4951a1c 100644
--- a/drivers/usb/host/uhci-debug.c
+++ b/drivers/usb/host/uhci-debug.c
@@ -16,6 +16,8 @@
 
 #include uhci-hcd.h
 
+#define LEFT_OUTPUT(1024)
+
 static struct dentry *uhci_debugfs_root;
 
 #ifdef DEBUG
@@ -44,10 +46,6 @@ static int uhci_show_td(struct uhci_hcd *uhci, struct 
uhci_td *td, char *buf,
char *spid;
u32 status, token;
 
-   /* Try to make sure there's enough memory */
-   if (len  160)
-   return 0;
-
status = td_status(uhci, td);
out += sprintf(out, %*s[%p] link (%08x) , space, , td,
hc32_to_cpu(uhci, td-link));
@@ -64,6 +62,8 @@ static int uhci_show_td(struct uhci_hcd *uhci, struct uhci_td 
*td, char *buf,
(status  TD_CTRL_CRCTIMEO) ? CRC/Timeo  : ,
(status  TD_CTRL_BITSTUFF) ? BitStuff  : ,
status  0x7ff);
+   if (out - buf  len)
+   goto truncate;
 
token = td_token(uhci, td);
switch (uhci_packetid(token)) {
@@ -90,7 +90,11 @@ static int uhci_show_td(struct uhci_hcd *uhci, struct 
uhci_td *td, char *buf,
spid);
out += sprintf(out, (buf=%08x)\n, hc32_to_cpu(uhci, td-buffer));
 
+tail:
return out - buf;
+truncate:
+   out += sprintf(out,  (%s truncated)\n, __FUNCTION__);
+   goto tail;
 }
 
 static int uhci_show_urbp(struct uhci_hcd *uhci, struct urb_priv *urbp,
@@ -101,8 +105,6 @@ static int uhci_show_urbp(struct uhci_hcd *uhci, struct 
urb_priv *urbp,
int i, nactive, ninactive;
char *ptype;
 
-   if (len  200)
-   return 0;
 
out += sprintf(out, urb_priv [%p] , urbp);
out += sprintf(out, urb [%p] , urbp-urb);
@@ -110,6 +112,8 @@ static int uhci_show_urbp(struct uhci_hcd *uhci, struct 
urb_priv *urbp,
out += sprintf(out, Dev=%d , usb_pipedevice(urbp-urb-pipe));
out += sprintf(out, EP=%x(%s) , usb_pipeendpoint(urbp-urb-pipe),
(usb_pipein(urbp-urb-pipe) ? IN : OUT));
+   if (out - buf  len)
+   goto truncate;
 
switch (usb_pipetype(urbp-urb-pipe)) {
case PIPE_ISOCHRONOUS: ptype = ISO; break;
@@ -128,6 +132,9 @@ static int uhci_show_urbp(struct uhci_hcd *uhci, struct 
urb_priv *urbp,
out += sprintf(out,  Unlinked=%d, urbp-urb-unlinked);
out += sprintf(out, \n);
 
+   if (out - buf  len)
+   goto truncate;
+
i = nactive = ninactive = 0;
list_for_each_entry(td, urbp-td_list, list) {
if (urbp-qh-type != USB_ENDPOINT_XFER_ISOC 
@@ -135,6 +142,8 @@ static int uhci_show_urbp(struct uhci_hcd *uhci, struct 
urb_priv *urbp,
out += sprintf(out, %*s%d: , space + 2, , i);
out += uhci_show_td(uhci, td, out,
len - (out - buf), 0);