linux-kernel - Re: [PATCH v2] scsi: pmcraid: Fix memory allocation in 'pmcraid_alloc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220110164417.GA7133@lst.de>
Date:   Mon, 10 Jan 2022 17:44:17 +0100
From:   Christoph Hellwig <hch@....de>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc:     hch@....de, "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        Bart Van Assche <bvanassche@....org>,
        Johannes Thumshirn <jthumshirn@...e.de>,
        Hannes Reinecke <hare@...e.com>, linux-kernel@...r.kernel.org,
        kernel-janitors@...r.kernel.org, linux-scsi@...r.kernel.org
Subject: Re: [PATCH v2] scsi: pmcraid: Fix memory allocation in
 'pmcraid_alloc_sglist()'

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@...adoo.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@....de>