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

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

[adam radford]

[-- Attachment #1: Type: text/plain, Size: 956 bytes --]

James/Linux-scsi,

The following patch from Bjorn Mork for megaraid_sas sanity checks the
user supplied length in
megasas_mgmt_fw_ioctl() to prevent a null pointer dereference in
dma_alloc_coherent() while running
smartmontools.

Signed-off-by: Adam Radford <aradford@gmail.com>

diff -Naur linux-2.6.38-rc5/drivers/scsi/megaraid/megaraid_sas_base.c
linux-2.6.38-rc5.new/drivers/scsi/megaraid/megaraid_sas_base.c
--- linux-2.6.38-rc5/drivers/scsi/megaraid/megaraid_sas_base.c	2011-02-19
14:07:13.455395141 -0800
+++ linux-2.6.38-rc5.new/drivers/scsi/megaraid/megaraid_sas_base.c	2011-02-19
14:08:31.914332199 -0800
@@ -4630,6 +4630,11 @@
 	 * 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 == 0) {
+			kbuff_arr[i] = NULL;
+			continue;
+		}
+
 		kbuff_arr[i] = dma_alloc_coherent(&instance->pdev->dev,
 						    ioc->sgl[i].iov_len,
 						    &buf_handle, GFP_KERNEL);

[-- Attachment #2: megaraid_sas.patch7 --]
[-- Type: application/octet-stream, Size: 676 bytes --]

diff -Naur linux-2.6.38-rc5/drivers/scsi/megaraid/megaraid_sas_base.c linux-2.6.38-rc5.new/drivers/scsi/megaraid/megaraid_sas_base.c
--- linux-2.6.38-rc5/drivers/scsi/megaraid/megaraid_sas_base.c	2011-02-19 14:07:13.455395141 -0800
+++ linux-2.6.38-rc5.new/drivers/scsi/megaraid/megaraid_sas_base.c	2011-02-19 14:08:31.914332199 -0800
@@ -4630,6 +4630,11 @@
 	 * 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 == 0) {
+			kbuff_arr[i] = NULL;
+			continue;
+		}
+
 		kbuff_arr[i] = dma_alloc_coherent(&instance->pdev->dev,
 						    ioc->sgl[i].iov_len,
 						    &buf_handle, GFP_KERNEL);

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

[Tomas Henzl]

On 02/20/2011 03:22 AM, adam radford wrote:
> James/Linux-scsi,
>
> The following patch from Bjorn Mork for megaraid_sas sanity checks the
> user supplied length in
> megasas_mgmt_fw_ioctl() to prevent a null pointer dereference in
> dma_alloc_coherent() while running
> smartmontools.
>   
Hi Adam,

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;
+

Tomas


> Signed-off-by: Adam Radford <aradford@gmail.com>
>
> diff -Naur linux-2.6.38-rc5/drivers/scsi/megaraid/megaraid_sas_base.c
> linux-2.6.38-rc5.new/drivers/scsi/megaraid/megaraid_sas_base.c
> --- linux-2.6.38-rc5/drivers/scsi/megaraid/megaraid_sas_base.c	2011-02-19
> 14:07:13.455395141 -0800
> +++ linux-2.6.38-rc5.new/drivers/scsi/megaraid/megaraid_sas_base.c	2011-02-19
> 14:08:31.914332199 -0800
> @@ -4630,6 +4630,11 @@
>  	 * 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 == 0) {
> +			kbuff_arr[i] = NULL;
> +			continue;
> +		}
> +
>  		kbuff_arr[i] = dma_alloc_coherent(&instance->pdev->dev,
>  						    ioc->sgl[i].iov_len,
>  						    &buf_handle, GFP_KERNEL);
>   

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

[adam radford]

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.

-Adam
--
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

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

[Bjørn Mork]

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