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@xxxxxxxxxx>
> ---
> 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@xxxxxx>
