[smartmontools-support] smartctl cciss segfault crash

[Christian Franke]

Hi,

Michal Hlavinka wrote:
> one of our users with hpe proliant g10 sees smartctl crash once a 
> while. I was able to check the core file and found a problem:
>
> ...
>
> cciss.cpp: cciss_io_interface(...) : 96
> int cciss_io_interface(int device, int target, struct scsi_cmnd_io * 
> iop, int report)
> {
>    unsigned char pBuf[512] = {0};
> ...
> ...
>    if (DXFER_FROM_DEVICE == iop->dxfer_dir)
>    {
> -->   memcpy(iop->dxferp, pBuf, iop->dxfer_len);
>       if (report > 1)
>              {
>
> where pBuf is a local buffer of 512 bytes
> iop->dxfer_len is 16124, it is initialized from pageLen value in 
> scsicmds.cpp:scsiLogSense:539:
>
> -->  io_hdr.dxfer_len = pageLen;
>
> int scsiLogSense(scsi_device * device, int pagenum, int subpagenum, 
> uint8_t *pBuf, int bufLen, int known_resp_len)
>
> with pageLen being bufLen because known_resp_len < 0 and bufLen = 16124
>
> 526: else if (known_resp_len < 0)
> 527:    pageLen = bufLen;
>
> That is obviously too much as cciss_io_interface uses its own pBuf and 
> not the one provided to scsiLogSense and cciss_io_interface's pBuf is 
> only 512 bytes thus it crashes.
> ...

That for the bug report and the detailed analysis. The small buffer is 
inherited from the early days of CCISS support (2006).

The attached quick hack patch should fix this. Not yet committed because 
I could not test is with the real hardware.

Thanks,
Christian

-------------- next part --------------
Index: cciss.cpp
===================================================================
--- cciss.cpp	(revision 4975)
+++ cciss.cpp	(working copy)
@@ -71,10 +71,12 @@
 */
 int cciss_io_interface(int device, int target, struct scsi_cmnd_io * iop, int report)
 {
-     unsigned char pBuf[512] = {0};
+     switch (iop->dxfer_dir) {
+        case DXFER_NONE: case DXFER_FROM_DEVICE: break;
+        default: return -ENOTSUP; // TODO: Support DXFER_TO_DEVICE
+     }
+
      unsigned char phylun[8] = {0};
-     int iBufLen = 512;
-     int len = 0; // used later in the code.
  
      int status = cciss_getlun(device, target, phylun, report);
      if (report > 0)
@@ -85,6 +87,10 @@
          return -ENXIO;      /* give up, assume no device there */
      }
 
+     unsigned char sensebuf[SEND_IOCTL_RESP_SENSE_LEN];
+     unsigned char * pBuf = (iop->dxferp ? iop->dxferp : sensebuf);
+     unsigned iBufLen = (iop->dxferp ? iop->dxfer_len : sizeof(sensebuf));
+
      status = cciss_sendpassthru( 2, iop->cmnd, iop->cmnd_len, (char*) pBuf, iBufLen, 1, phylun, device);
  
      if (0 == status)
@@ -93,7 +99,6 @@
              printf("  status=0\n");
          if (DXFER_FROM_DEVICE == iop->dxfer_dir)
          {
-             memcpy(iop->dxferp, pBuf, iop->dxfer_len);
              if (report > 1)
              {
                  int trunc = (iop->dxfer_len > 256) ? 1 : 0;
@@ -107,13 +112,15 @@
      iop->scsi_status = status & 0x7e; /* bits 0 and 7 used to be for vendors */
      if (LSCSI_DRIVER_SENSE == ((status >> 24) & 0xf))
          iop->scsi_status = SCSI_STATUS_CHECK_CONDITION;
-     len = (SEND_IOCTL_RESP_SENSE_LEN < iop->max_sense_len) ?
-                SEND_IOCTL_RESP_SENSE_LEN : iop->max_sense_len;
+     unsigned len = (SEND_IOCTL_RESP_SENSE_LEN < iop->max_sense_len) ?
+                     SEND_IOCTL_RESP_SENSE_LEN : iop->max_sense_len;
+     if (len > iBufLen)
+       len = iBufLen;
      if ((SCSI_STATUS_CHECK_CONDITION == iop->scsi_status) &&
          iop->sensep && (len > 0))
      {
          memcpy(iop->sensep, pBuf, len);
-         iop->resp_sense_len = iBufLen;
+         iop->resp_sense_len = len;
          if (report > 1)
          {
              printf("  >>> Sense buffer, len=%d:\n", (int)len);
@@ -173,7 +180,7 @@
     iocommand.Request.CDBLen = CDBlen;
     iocommand.Request.Type.Type = TYPE_CMD;
     iocommand.Request.Type.Attribute = ATTR_SIMPLE;
-    iocommand.Request.Type.Direction = XFER_READ;
+    iocommand.Request.Type.Direction = XFER_READ; // TODO: OK for DXFER_NONE ?
     iocommand.Request.Timeout = 0;
 
     iocommand.buf_size = size;