Re: [PATCH for 5.10.x 1/2] swiotlb: fix info leak with DMA_FROM_DEVICE

[Halil Pasic <pasic@linux.ibm.com>]

On Tue, 22 Mar 2022 11:36:40 +0100
Greg KH <gregkh@linuxfoundation.org> wrote:

> On Tue, Mar 22, 2022 at 11:28:34AM +0100, Halil Pasic wrote:
> > On Tue, 22 Mar 2022 11:10:01 +0100
> > Greg KH <gregkh@linuxfoundation.org> wrote:
> >   
> > > On Tue, Mar 22, 2022 at 11:02:17AM +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.
> > > > 
> > > > One can argue that this is an swiotlb problem, because without swiotlb
> > > > we leak all zeros, and the swiotlb should be transparent in a sense that
> > > > it does not affect the outcome (if all other participants are well
> > > > behaved).
> > > > 
> > > > 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 if in doubt, but allow the driver
> > > > to tell us that the whole mapped buffer is going to be overwritten,
> > > > in which case we can preserve the old behavior and avoid the performance
> > > > impact of the extra bounce.
> > > > 
> > > > Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> > > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > > Cc: stable@vger.kernel.org
> > > > [pasic@linux.ibm.com: resolved merge conflicts]
> > > > ---
> > > >  Documentation/core-api/dma-attributes.rst | 8 ++++++++
> > > >  include/linux/dma-mapping.h               | 8 ++++++++
> > > >  kernel/dma/swiotlb.c                      | 3 ++-
> > > >  3 files changed, 18 insertions(+), 1 deletion(-)    
> > > 
> > > What is the git commit id of this patch in Linus's tree?  
> > 
> > ddbd89deb7d3 ("swiotlb: fix info leak with DMA_FROM_DEVICE")
> > 
> > What is the best way to state the original commit id for backports? I
> > used the cover letter this time, but it does not seem to be the right
> > choice.  
> 
> Below the --- line is fine, or somewhere that I can see it in the patch,
> much like we do for the commits in the stable trees is even better.

Thanks! I will go with
" commit <SHA> upstream."
line after the short description then.

Regards,
Halil