Re: [PATCH] [SCSI] megaraid_sas: Sanity check user supplied length before passing it to dma_alloc_coherent()

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

FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> writes:

> The patch looks fine to me. dma_alloc_coherent() doesn't take zero for
> the size argument (causes a kernel crash). The driver can't assume
> that an applications is sane so it needs to check the size that an
> application passed on.
>
> Unfortunately, your patch can't be applied to the latest git. I think
> that you need to submit the updated patch first. After it's merged,
> you can send stable maintainers the modified patch that can be applied
> to stable kernels.
>
> Btw, about your patch, it's better to use "if (!hoge)" instead of "if
> (hoge == 0)" 

I believe that is a matter of taste, although I tend to agree that it
looks better.  I used the "(hoge == 0)" syntax to try to keep in line
with the style already used in this driver, like e.g.

static int
megasas_queue_command_lck(struct scsi_cmnd *scmd, void (*done) (struct scsi_cmnd *))
{
	struct megasas_instance *instance;
	unsigned long flags;

	instance = (struct megasas_instance *)
	    scmd->device->host->hostdata;

	if (instance->issuepend_done == 0)
		return SCSI_MLQUEUE_HOST_BUSY;



but I see now that there are quite a few "if (!hoge)" as well, so I will
update as you suggest.


> and kbuff_arr[] is initialized so seems that you don't
> need to reset it again.

Thanks.  Don't understand how I could have missed that.


> The updated patch would be something like:
>
> diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c b/drivers/scsi/megaraid/megaraid_sas_base.c
> index 5d6d07b..cee1d3b 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> @@ -4611,6 +4611,9 @@ 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)
> +			continue;
> +
>  		kbuff_arr[i] = dma_alloc_coherent(&instance->pdev->dev,
>  						    ioc->sgl[i].iov_len,
>  						    &buf_handle, GFP_KERNEL);


Yes, I'll followup with that in a separate mail.  Thanks a lot for your
thorough review.



Bjørn
--
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