[PATCH v2] scsi: pmcraid: Fix memory allocation in 'pmcraid_alloc_sglist()'

[PATCH v2] scsi: pmcraid: Fix memory allocation in 'pmcraid_alloc_sglist()'

[Christophe JAILLET]

When the scatter list is allocated in 'pmcraid_alloc_sglist()', the
corresponding pointer should be stored in 'scatterlist' within the
'pmcraid_sglist' structure. Otherwise, 'scatterlist' is NULL.

This leads to a potential memory leak and NULL pointer dereference.

Fixes: ed4414cef2ad ("scsi: pmcraid: Use sgl_alloc_order() and sgl_free_order()")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
This patch is completely speculative and untested.

Should it be correct, I think that their should be some trouble somewhere.
Either NULL pointer dereference or incorrect behavior.
The patch that introduced this potential bug is from 2018-02. So, this
should have been spotted earlier.

So unless this driver is mostly unused, this looks odd to me.
Feedback appreciated.

Review with care!

v2: synch with -next-20220110
---
 drivers/scsi/pmcraid.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/pmcraid.c b/drivers/scsi/pmcraid.c
index 928532180d32..e314ea133827 100644
--- a/drivers/scsi/pmcraid.c
+++ b/drivers/scsi/pmcraid.c
@@ -3221,8 +3221,9 @@ static struct pmcraid_sglist *pmcraid_alloc_sglist(int buflen)
 		return NULL;
 
 	sglist->order = order;
-	sgl_alloc_order(buflen, order, false, GFP_KERNEL | __GFP_ZERO,
-			&sglist->num_sg);
+	sglist->scatterlist = sgl_alloc_order(buflen, order, false,
+					      GFP_KERNEL | __GFP_ZERO,
+					      &sglist->num_sg);
 
 	return sglist;
 }
-- 
2.32.0

Re: [PATCH v2] scsi: pmcraid: Fix memory allocation in 'pmcraid_alloc_sglist()'

[Christoph Hellwig]

On Mon, Jan 10, 2022 at 01:02:53PM +0100, Christophe JAILLET wrote:
> When the scatter list is allocated in 'pmcraid_alloc_sglist()', the
> corresponding pointer should be stored in 'scatterlist' within the
> 'pmcraid_sglist' structure. Otherwise, 'scatterlist' is NULL.
> 
> This leads to a potential memory leak and NULL pointer dereference.
> 
> Fixes: ed4414cef2ad ("scsi: pmcraid: Use sgl_alloc_order() and sgl_free_order()")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> This patch is completely speculative and untested.
> 
> Should it be correct, I think that their should be some trouble somewhere.
> Either NULL pointer dereference or incorrect behavior.
> The patch that introduced this potential bug is from 2018-02. So, this
> should have been spotted earlier.
> 
> So unless this driver is mostly unused, this looks odd to me.
> Feedback appreciated.

The whole passthrough ioctl path looks completely broken to me.  For
example it dma maps the scatterlist and after that copies data to it,
which is prohibited by the DMA API contract.

So I'd be tempted to just remove the PMCRAID_PASSTHROUGH_IOCTL ioctl
implementation entirely, and if users for it do pop up we should
reimplement it using the proper block layer request mapping helpers.

If for some reason we don't want that and just fix the obvious
problem without a way to test for it, your patch looks good to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2] scsi: pmcraid: Fix memory allocation in 'pmcraid_alloc_sglist()'

[Martin K. Petersen]

Christoph,

> The whole passthrough ioctl path looks completely broken to me.  For
> example it dma maps the scatterlist and after that copies data to it,
> which is prohibited by the DMA API contract.
>
> So I'd be tempted to just remove the PMCRAID_PASSTHROUGH_IOCTL ioctl
> implementation entirely, and if users for it do pop up we should
> reimplement it using the proper block layer request mapping helpers.

Given that nobody has complained since 2018 I propose we remove it.

-- 
Martin K. Petersen	Oracle Linux Engineering