re: [SCSI] esas2r: ATTO Technology ExpressSAS 6G SAS/SATA RAID Adapter Driver

re: [SCSI] esas2r: ATTO Technology ExpressSAS 6G SAS/SATA RAID Adapter Driver

[Colin Ian King]

Hi,

Static analysis wit Coverity has found an issue in the following commit:

commit 26780d9e12edf45c0b98315de272b1feff5a8e93
Author: Bradley Grove <bgrove@attotech.com>
Date:   Fri Aug 23 10:35:45 2013 -0400

    [SCSI] esas2r: ATTO Technology ExpressSAS 6G SAS/SATA RAID Adapter
Driver

The issue is in function write_fs in drivers/scsi/esas2r/esas2r_main.c
as follows:

101        int result = 0;
102
103        result = esas2r_write_fs(a, buf, off, count);
104
105        if (result < 0)

Unused value (UNUSED_VALUE) assigned_value: Assigning value 0 to result
here, but that stored value is not used.

106                result = 0;
107
108        return length;

I'm not sure what the intention was for this. Was length meant to be
assigned to 0 rather than result?  Or is the result < 0 check just
unnecessary code?

Colin

re: [SCSI] esas2r: ATTO Technology ExpressSAS 6G SAS/SATA RAID Adapter Driver

[Dan Carpenter]

Hello Bradley Grove,

The patch 26780d9e12ed: "[SCSI] esas2r: ATTO Technology ExpressSAS 6G
SAS/SATA RAID Adapter Driver" from Aug 23, 2013, leads to the
following static checker warning:

	drivers/scsi/esas2r/esas2r_ioctl.c:1902 esas2r_read_vda()
	error: 'count' from user is not capped properly

drivers/scsi/esas2r/esas2r_ioctl.c
  1892  
  1893          if (off > VDA_MAX_BUFFER_SIZE)
  1894                  return 0;
  1895  
  1896          if (count + off > VDA_MAX_BUFFER_SIZE)
                    ^^^^^
"count" is a user controlled int.  Let's assume we're on a 32 system for
simplicity.  If count is a high positive number here, then count + off
is negative and thus less than VDA_MAX_BUFFER_SIZE.

  1897                  count = VDA_MAX_BUFFER_SIZE - off;
  1898  
  1899          if (count < 0)
  1900                  return 0;
  1901  
  1902          memcpy(buf, a->vda_buffer + off, count);
                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Memory corruption.

  1903  
  1904          return count;
  1905  }

"count" comes from the ioctl.  Let's look at that:

drivers/scsi/esas2r/esas2r_ioctl.c
  1476          case EXPRESS_IOCTL_VDA:
  1477                  err = esas2r_write_vda(a,
  1478                                         (char *)&ioctl->data.ioctl_vda,
  1479                                         0,
  1480                                         sizeof(struct atto_ioctl_vda) +
  1481                                         ioctl->data.ioctl_vda.data_length);
                                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  1482  
  1483                  if (err >= 0) {
  1484                          err = esas2r_read_vda(a,
  1485                                                (char *)&ioctl->data.ioctl_vda,
  1486                                                0,
  1487                                                sizeof(struct atto_ioctl_vda) +
  1488                                                ioctl->data.ioctl_vda.data_length);
                                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
These additions have integer overflow bugs.  It seems harmless to me,
but hopefully static checkers will eventually start complaining about
them.

  1489                  }
  1490  
  1491  
  1492  
  1493  
  1494                  break;

regards,
dan carpenter

Re: [SCSI] esas2r: ATTO Technology ExpressSAS 6G SAS/SATA RAID Adapter Driver

[Dave Jones]

 > +struct esas2r_adapter {
 > +	struct esas2r_target targetdb[ESAS2R_MAX_TARGETS];
 > +	struct esas2r_target *targetdb_end;
 ... 
 > +	u8 fw_coredump_buff[ESAS2R_FWCOREDUMP_SZ];


 > +void esas2r_reset_chip(struct esas2r_adapter *a)
 > +{
 > +	if (!esas2r_is_adapter_present(a))
 > +		return;
 > +
 > +	/*
 > +	 * Before we reset the chip, save off the VDA core dump.  The VDA core
 > +	 * dump is located in the upper 512KB of the onchip SRAM.  Make sure
 > +	 * to not overwrite a previous crash that was saved.
 > +	 */
 > +	if ((a->flags2 & AF2_COREDUMP_AVAIL)
 > +	    && !(a->flags2 & AF2_COREDUMP_SAVED)
 > +	    && a->fw_coredump_buff) {
 > +		esas2r_read_mem_block(a,
 > +				      a->fw_coredump_buff,
 > +				      MW_DATA_ADDR_SRAM + 0x80000,
 > +				      ESAS2R_FWCOREDUMP_SZ);

Comparing an array (fw_coredump_buff) to null probably isn't what you intended here.

	Dave

re: [SCSI] esas2r: ATTO Technology ExpressSAS 6G SAS/SATA RAID Adapter Driver

[Dan Carpenter]

Hello Bradley Grove,

The patch 17adeb6dabbe: "[SCSI] esas2r: ATTO Technology ExpressSAS 6G
SAS/SATA RAID Adapter Driver" from Aug 23, 2013, leads to the
following Smatch warning:
"drivers/scsi/esas2r/esas2r_vda.c:312 esas2r_complete_vda_ioctl()
	 error: format string overflow. buf_size: 4 length: 5"

drivers/scsi/esas2r/esas2r_vda.c
   312                          sprintf((char *)&cfg->data.init.fw_release,
                                                 ^^^^^^^^^^^^^^^^^^^^^^^^^
This is a u32 but we are writing 4 characters and a NUL so it ends up
putting the NUL in cfg->data.init.epoch_time.

   313                                  "%1d.%02d",
   314                                  (int)LOBYTE(le16_to_cpu(rsp->fw_release)),
   315                                  (int)HIBYTE(le16_to_cpu(rsp->fw_release)));

regards,
dan carpenter

re: [SCSI] esas2r: ATTO Technology ExpressSAS 6G SAS/SATA RAID Adapter Driver

[Dan Carpenter]

Hello Bradley Grove,

This is a semi-automatic email about new static checker warnings.

The patch 17adeb6dabbe: "[SCSI] esas2r: ATTO Technology ExpressSAS 6G 
SAS/SATA RAID Adapter Driver" from Aug 23, 2013, leads to the 
following Smatch complaint:

drivers/scsi/esas2r/esas2r_init.c:671 esas2r_cleanup()
	 warn: variable dereferenced before check 'host' (see line 668)

drivers/scsi/esas2r/esas2r_init.c
   667	{
   668		struct esas2r_adapter *a = (struct esas2r_adapter *)host->hostdata;
                                                                    ^^^^^^^^^^^^^^
Patch adds dereference.

   669		int index;
   670	
   671		if (host == NULL) {
                    ^^^^^^^^^^^^
Patch adds check.

   672			int i;
   673	

regards,
dan carpenter