Re: DMA error when sg->offset value is greater than PAGE_SIZE in Intel IOMMU

[Raj, Ashok]

Hi Robin

On Wed, Sep 27, 2017 at 06:18:02PM +0100, Robin Murphy wrote:
> On Wed, 27 Sep 2017 16:31:04 +0000
> Casey Leedom <leedom@xxxxxxxxxxx> wrote:
> 
> > | From: Dan Williams <dan.j.williams@xxxxxxxxx>
> > | Sent: Tuesday, September 26, 2017 9:10 AM
> > |   
> > | On Tue, Sep 26, 2017 at 9:06 AM, Casey Leedom <leedom@xxxxxxxxxxx>
> > wrote: | > | From: Robin Murphy <robin.murphy@xxxxxxx>
> > | > | Sent: Tuesday, September 26, 2017 7:22 AM
> > | > |...
> > | > ...
> > | >   Regardless, it seems that you agree that there's an issue with
> > the Intel | > I/O MMU support code with regard to the legal values
> > which a (struct | > scatterlist) can take on?  I still can't find any
> > documentation for this | > and, personally, I'm a bit baffled by a
> > Page-oriented Scatter/Gather List | > representation where [Offset,
> > Offset+Length) can reside outside the Page. |
> > | Consider the case where the page represents a huge page, then an
> > | offset greater than PAGE_SIZE (up to HPAGE_SIZE) makes sense.
> > 
> >   Okay, but whatever the underlaying Page Size is, should [Offset,
> > Offset+Length) completely reside within the referenced Page?  I'm just
> > trying to understand the Invariance Conditions which are assumed by
> > all of the code which processes Scatter/gather Lists ...
> 
> From my experience, in general terms each scatterlist segment
> represents some contiguous quantity of pages, of which sg->page is the
> first, while sg->length and sg->offset describe the specific bounds of
> that segment's data. As such, the length may certainly (and frequently
> does) exceed PAGE_SIZE; for the offset, it's unlikely that the producer
> would initially construct one greater than PAGE_SIZE instead of just
> pointing sg->page further forward, but it seems reasonable for it to
> come about if some intermediate subsystem is processing an existing
> list in-place (as seems to be the case with crypto here).
> 
> My opinion is that this may be a slightly unusual case, but I would
> not consider it an illegal one. I think most DMA mapping
> implementations would handle it whether intentionally or not.

In this specific case, it appears that 

scatterwalk_ffwd()->sg_set_page()

sg_set_page(dst, sg_page(src), src->length - len, src->offset + len);

and 

static inline void sg_set_page(struct scatterlist *sg, struct page *page,
                   unsigned int len, unsigned int offset)
{
    sg_assign_page(sg, page);
    sg->offset = offset;
    sg->length = len;
}

The src->offset + len seems to be the culprit putting it past the page.
Looks like in the cases when it breaks, the offset is already towards
the end of page.. and adding the len, puts it over the limit.

When dealing with the offset > PAGE_SIZE, is the expectation you have another
additional entry for sgl? for e.g.

if sg->page = X, and offset=4092. and len = 16. Since IOMMU only understands
4K pages this last entry needs to be adjusted?

I'm not sure if the offset+len is a buffer overflow situation or just 
trips IOMMU. 

Cheers,
Ashok

the scatter gather list, should we