Re: [Iscsitarget-devel] [Patch 1/1] Segfault in ietd buffer overrun indump_line

[Shreyansh J. <shr...@gm...>]

Hi,

Following patch fixes a buffer overrun problem in __dump_line function
in log.c file. It also removes a stray 'return' call in log_pdu
function which was restricting a PDU dump call to complete log_pdu operation.

In dump_line, the array line[] was defined as line[53] whereas sprintf
was pushing in 59 characters into it. This was leading to segfault of
ietd when debugging was enabled. Through this patch, the size of line
char array would be linked to the size of buffer defined in log_pdu
function. The calculation used is (N*3 + ((N/4)+1)*2 + 3), where N is
the expected size of buffer used in log_pdu function.

Also, in log_pdu function, the first check for log_level against
passed parameter for log was incorrect and would have forced a return
even when the caller has set the logging level to acceptable level.

---
I have verified the patch by forcing the four entry points of this
function (in ietd.c and iscsid.c) to use this function, and also by
modifying the size of the buffer being used. In case someone feels
that more testcases may have been left out, please let me know. All
others comments/suggestions are also welcome.
---
Signed-off-by: Shreyansh Jain <shrey.linux at gmail.com>

--------BEGIN---------
Index: usr/log.c
===================================================================
--- usr/log.c   (revision 168)
+++ usr/log.c   (working copy)
@@ -62,15 +62,26 @@
       }
 }

+/* Definition for log_pdu buffer */
+#define        BUFFER_SIZE             16
+/* Buffer line would be printed in the following format in syslog
+ * ' aa bb cc dd | ee ff gg hh | ii jj kk ll | ............... |'
+ * Means, 61 Characters if size of line is 16 chars. For N being the buffer
+ * size, the fomula would be  = (N*3 + ((N/4)+1)*2 + 3), where, the middle term
+ * is to counter printing " | " after each 4 bytes. Last '3' is to compensate
+ * for "   " printed when (i >= cnt) (and when will that happen??)
+ */
+#define        LINE_SIZE       (((BUFFER_SIZE)*3) +
((((int)BUFFER_SIZE/4)+1)*2) + 3)
+
 static void __dump_line(int level, unsigned char *buf, int *cp)
 {
-       char line[16*3+5], *lp = line;
+       char line[LINE_SIZE], *lp = line;
       int i, cnt;

       cnt = *cp;
       if (!cnt)
               return;
-       for (i = 0; i < 16; i++) {
+       for (i = 0; i < BUFFER_SIZE; i++) {
               if (i < cnt)
                       lp += sprintf(lp, " %02x", buf[i]);
               else
@@ -89,7 +100,7 @@
       int cnt = (*cp)++;

       buf[cnt] = ch;
-       if (cnt == 15)
+       if (cnt == (BUFFER_SIZE - 1))
               __dump_line(level, buf, cp);
 }

@@ -98,13 +109,12 @@

 void log_pdu(int level, struct PDU *pdu)
 {
-       unsigned char char_buf[16];
+       unsigned char char_buf[BUFFER_SIZE];
       int char_cnt = 0;
       unsigned char *buf;
       int i;
-       return;

-       if (log_level <= level)
+       if (log_level < level)
               return;

       buf = (void *)&pdu->bhs;

--------END---------