Re: [PATCH 2/2] swiotlb: fix info leak with DMA_FROM_DEVICE

[Greg KH <greg@kroah.com>]

On Fri, Mar 04, 2022 at 02:58:59PM +0100, Halil Pasic wrote:
> The problem I'm addressing was discovered by the LTP test covering
> cve-2018-1000204.
> 
> A short description of what happens follows:
> 1) The test case issues a command code 00 (TEST UNIT READY) via the SG_IO
>    interface with: dxfer_len == 524288, dxdfer_dir == SG_DXFER_FROM_DEV
>    and a corresponding dxferp. The peculiar thing about this is that TUR
>    is not reading from the device.
> 2) In sg_start_req() the invocation of blk_rq_map_user() effectively
>    bounces the user-space buffer. As if the device was to transfer into
>    it. Since commit a45b599ad808 ("scsi: sg: allocate with __GFP_ZERO in
>    sg_build_indirect()") we make sure this first bounce buffer is
>    allocated with GFP_ZERO.
> 3) For the rest of the story we keep ignoring that we have a TUR, so the
>    device won't touch the buffer we prepare as if the we had a
>    DMA_FROM_DEVICE type of situation. My setup uses a virtio-scsi device
>    and the  buffer allocated by SG is mapped by the function
>    virtqueue_add_split() which uses DMA_FROM_DEVICE for the "in" sgs (here
>    scatter-gather and not scsi generics). This mapping involves bouncing
>    via the swiotlb (we need swiotlb to do virtio in protected guest like
>    s390 Secure Execution, or AMD SEV).
> 4) When the SCSI TUR is done, we first copy back the content of the second
>    (that is swiotlb) bounce buffer (which most likely contains some
>    previous IO data), to the first bounce buffer, which contains all
>    zeros.  Then we copy back the content of the first bounce buffer to
>    the user-space buffer.
> 5) The test case detects that the buffer, which it zero-initialized,
>   ain't all zeros and fails.
> 
> This is an swiotlb problem, because the swiotlb should be transparent in
> a sense that it does not affect the outcome (if all other participants
> are well behaved), and without swiotlb we leak all zeros.  Even if
> swiotlb_tbl_map_single() zero-initialised the allocated slot(s) to avoid
> leaking stale data back to the caller later, when it comes to unmap or
> sync_for_cpu it still fundamentally cannot tell how much of the contents
> of the bounce slot have actually changed, therefore if the caller was
> expecting the device to do a partial write, the rest of the mapped
> buffer *will* be corrupted by bouncing the whole thing back again.
> 
> Copying the content of the original buffer into the swiotlb buffer is
> the only way I can think of to make swiotlb transparent in such
> scenarios. So let's do just that.
> 
> The extra bounce is expected to hurt the performance. For the cases
> where the extra bounce is not necessary we could get rid of it, if we
> were told by the client code, that it is not needed. Such optimisations
> are out of scope for this patch, and are likely to be a subject of some
> future work.
> 
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> Reported-by: Ali Haider <ali.haider@ibm.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
>  kernel/dma/swiotlb.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>