| Message ID | 11a1bc98501de37baa5bcd10b61136f6e450b82e.1641816080.git.christophe.jaillet@wanadoo.fr |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] scsi: pmcraid: Fix memory allocation in 'pmcraid_alloc_sglist()' | expand |
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>
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.
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; }
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(-)