Re: [PATCH 7/15] megaraid_sas: Sanity check user supplied length in megasas_mgmt_fw_ioctl

["Bjørn Mork" <bjorn@mork.no>]

adam radford <aradford@gmail.com> writes:
> On Thu, Feb 24, 2011 at 6:01 AM, Tomas Henzl <thenzl@redhat.com> wrote:
>>
>> there was proposed another patch for this issue -
>> http://marc.info/?l=linux-scsi&m=129542474703680&w=2
>> I think it's a little bit more precise.
>>
>>        for (i = 0; i < ioc->sge_count; i++) {
>> +               if (!ioc->sgl[i].iov_len)
>> +                       continue;
>> +
>>
>
> I didn't see this was already in scsi-misc-2.6.  I will re-base my
> patch series and repost.

Ben Hutchings noted as well in
http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=604627#47
that this removes the initialisation of kern_sge32[i] when
ioc->sgl[i].iov_len is zero.

Inspecting smartmontools shows that it is not an issue with them, as
they always sge32[0].length = sgl[0].iov_len before issuing the
passthrough command:

/* Issue passthrough scsi command to PERC5/6 controllers */
bool linux_megaraid_device::megasas_cmd(int cdbLen, void *cdb, 
  int dataLen, void *data,
  int /*senseLen*/, void * /*sense*/, int /*report*/)
{
  struct megasas_pthru_frame    *pthru;
  struct megasas_iocpacket      uio;
  int rc;

  memset(&uio, 0, sizeof(uio));
  pthru = (struct megasas_pthru_frame *)uio.frame.raw;
  pthru->cmd = MFI_CMD_PD_SCSI_IO;
  pthru->cmd_status = 0xFF;
  pthru->scsi_status = 0x0;
  pthru->target_id = m_disknum;
  pthru->lun = 0;
  pthru->cdb_len = cdbLen;
  pthru->timeout = 0;
  pthru->flags = MFI_FRAME_DIR_READ;
  pthru->sge_count = 1;
  pthru->data_xfer_len = dataLen;
  pthru->sgl.sge32[0].phys_addr = (intptr_t)data;
  pthru->sgl.sge32[0].length = (uint32_t)dataLen;
  memcpy(pthru->cdb, cdb, cdbLen);

  uio.host_no = m_hba;
  uio.sge_count = 1;
  uio.sgl_off = offsetof(struct megasas_pthru_frame, sgl);
  uio.sgl[0].iov_base = data;
  uio.sgl[0].iov_len = dataLen;

  rc = 0;
  errno = 0;
  rc = ioctl(m_fd, MEGASAS_IOC_FIRMWARE, &uio);





But to be utterly safe against userspace stupidity, the patch should
probably always set the kern_sge32[i].length.  Something like this,
maybe: 



diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
index 5d6d07b..4cc65fd 100644
--- a/drivers/scsi/megaraid/megaraid_sas_base.c
+++ b/drivers/scsi/megaraid/megaraid_sas_base.c
@@ -4611,6 +4611,11 @@ megasas_mgmt_fw_ioctl(struct megasas_instance *instance,
         * For each user buffer, create a mirror buffer and copy in
         */
        for (i = 0; i < ioc->sge_count; i++) {
+               if (!ioc->sgl[i].iov_len) {
+                       kern_sge32[i].length = 0;
+                       continue;
+               }
+
                kbuff_arr[i] = dma_alloc_coherent(&instance->pdev->dev,
                                                    ioc->sgl[i].iov_len,
                                                    &buf_handle, GFP_KERNEL);
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html