[PATCH] iommu/dma: Fix calculation overflow in __finalise_sg()

[PATCH] iommu/dma: Fix calculation overflow in __finalise_sg()

[Nicolin Chen]

The max_len is a u32 type variable so the calculation on the
left hand of the last if-condition will potentially overflow
when a cur_len gets closer to UINT_MAX -- note that there're
drivers setting max_seg_size to UINT_MAX:
  drivers/dma/dw-edma/dw-edma-core.c:745:
    dma_set_max_seg_size(dma->dev, U32_MAX);
  drivers/dma/dma-axi-dmac.c:871:
    dma_set_max_seg_size(&pdev->dev, UINT_MAX);
  drivers/mmc/host/renesas_sdhi_internal_dmac.c:338:
    dma_set_max_seg_size(dev, 0xffffffff);
  drivers/nvme/host/pci.c:2520:
    dma_set_max_seg_size(dev->dev, 0xffffffff);

So this patch just casts the cur_len in the calculation to a
size_t type to fix the overflow issue, as it's not necessary
to change the type of cur_len after all.

Fixes: 809eac54cdd6 ("iommu/dma: Implement scatterlist segment merging")
Cc: stable@vger.kernel.org
Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
---
 drivers/iommu/dma-iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index a9f13313a22f..676b7ecd451e 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -764,7 +764,7 @@ static int __finalise_sg(struct device *dev, struct scatterlist *sg, int nents,
 		 * - and wouldn't make the resulting output segment too long
 		 */
 		if (cur_len && !s_iova_off && (dma_addr & seg_mask) &&
-		    (cur_len + s_length <= max_len)) {
+		    ((size_t)cur_len + s_length <= max_len)) {
 			/* ...then concatenate it with the previous one */
 			cur_len += s_length;
 		} else {
-- 
2.17.1

Re: [PATCH] iommu/dma: Fix calculation overflow in __finalise_sg()

[Joerg Roedel]

On Fri, Jun 21, 2019 at 09:38:14PM -0700, Nicolin Chen wrote:
> The max_len is a u32 type variable so the calculation on the
> left hand of the last if-condition will potentially overflow
> when a cur_len gets closer to UINT_MAX -- note that there're
> drivers setting max_seg_size to UINT_MAX:
>   drivers/dma/dw-edma/dw-edma-core.c:745:
>     dma_set_max_seg_size(dma->dev, U32_MAX);
>   drivers/dma/dma-axi-dmac.c:871:
>     dma_set_max_seg_size(&pdev->dev, UINT_MAX);
>   drivers/mmc/host/renesas_sdhi_internal_dmac.c:338:
>     dma_set_max_seg_size(dev, 0xffffffff);
>   drivers/nvme/host/pci.c:2520:
>     dma_set_max_seg_size(dev->dev, 0xffffffff);
> 
> So this patch just casts the cur_len in the calculation to a
> size_t type to fix the overflow issue, as it's not necessary
> to change the type of cur_len after all.
> 
> Fixes: 809eac54cdd6 ("iommu/dma: Implement scatterlist segment merging")
> Cc: stable@vger.kernel.org
> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>

Looks good to me, but I let Robin take a look too before I apply it,
Robin?

Re: [PATCH] iommu/dma: Fix calculation overflow in __finalise_sg()

[Robin Murphy]

On 01/07/2019 13:21, Joerg Roedel wrote:
> On Fri, Jun 21, 2019 at 09:38:14PM -0700, Nicolin Chen wrote:
>> The max_len is a u32 type variable so the calculation on the
>> left hand of the last if-condition will potentially overflow
>> when a cur_len gets closer to UINT_MAX -- note that there're
>> drivers setting max_seg_size to UINT_MAX:
>>    drivers/dma/dw-edma/dw-edma-core.c:745:
>>      dma_set_max_seg_size(dma->dev, U32_MAX);
>>    drivers/dma/dma-axi-dmac.c:871:
>>      dma_set_max_seg_size(&pdev->dev, UINT_MAX);
>>    drivers/mmc/host/renesas_sdhi_internal_dmac.c:338:
>>      dma_set_max_seg_size(dev, 0xffffffff);
>>    drivers/nvme/host/pci.c:2520:
>>      dma_set_max_seg_size(dev->dev, 0xffffffff);
>>
>> So this patch just casts the cur_len in the calculation to a
>> size_t type to fix the overflow issue, as it's not necessary
>> to change the type of cur_len after all.
>>
>> Fixes: 809eac54cdd6 ("iommu/dma: Implement scatterlist segment merging")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> 
> Looks good to me, but I let Robin take a look too before I apply it,
> Robin?
I'll need to take a closer look at how exactly an overflow would happen 
here (just got back off some holiday), but my immediate thought is that 
if this is a real problem, then what about 32-bit builds where size_t 
would still overflow?

Robin.

Re: [PATCH] iommu/dma: Fix calculation overflow in __finalise_sg()

[Nicolin Chen]

Hi Robin,

On Mon, Jul 01, 2019 at 01:39:55PM +0100, Robin Murphy wrote:
> > > The max_len is a u32 type variable so the calculation on the
> > > left hand of the last if-condition will potentially overflow
> > > when a cur_len gets closer to UINT_MAX -- note that there're
> > > drivers setting max_seg_size to UINT_MAX:
> > >    drivers/dma/dw-edma/dw-edma-core.c:745:
> > >      dma_set_max_seg_size(dma->dev, U32_MAX);
> > >    drivers/dma/dma-axi-dmac.c:871:
> > >      dma_set_max_seg_size(&pdev->dev, UINT_MAX);
> > >    drivers/mmc/host/renesas_sdhi_internal_dmac.c:338:
> > >      dma_set_max_seg_size(dev, 0xffffffff);
> > >    drivers/nvme/host/pci.c:2520:
> > >      dma_set_max_seg_size(dev->dev, 0xffffffff);
> > > 
> > > So this patch just casts the cur_len in the calculation to a
> > > size_t type to fix the overflow issue, as it's not necessary
> > > to change the type of cur_len after all.
> > > 
> > > Fixes: 809eac54cdd6 ("iommu/dma: Implement scatterlist segment merging")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
> > 
> > Looks good to me, but I let Robin take a look too before I apply it,
> > Robin?
> I'll need to take a closer look at how exactly an overflow would happen here

It was triggered by a test case that was trying to map a 4GB
dma_buf (1000+ nents in the scatterlist). This function then
seemed to reduce the nents by merging most of them, probably
because they were contiguous.

> (just got back off some holiday), but my immediate thought is that if this
> is a real problem, then what about 32-bit builds where size_t would still
> overflow?

I think most of callers are also using size_t type for their
size parameters like dma_buf, so the cur_len + s_length will
unlikely go higher than UINT_MAX. But just in case that some
driver allocates a large sg with a size parameter defined in
64-bit and uses this map() function, so it might be safer to
change to "size_t" here to "u64"?

Thank you
Nicolin

Re: [PATCH] iommu/dma: Fix calculation overflow in __finalise_sg()

[Robin Murphy]

On 01/07/2019 22:50, Nicolin Chen wrote:
> Hi Robin,
> 
> On Mon, Jul 01, 2019 at 01:39:55PM +0100, Robin Murphy wrote:
>>>> The max_len is a u32 type variable so the calculation on the
>>>> left hand of the last if-condition will potentially overflow
>>>> when a cur_len gets closer to UINT_MAX -- note that there're
>>>> drivers setting max_seg_size to UINT_MAX:
>>>>     drivers/dma/dw-edma/dw-edma-core.c:745:
>>>>       dma_set_max_seg_size(dma->dev, U32_MAX);
>>>>     drivers/dma/dma-axi-dmac.c:871:
>>>>       dma_set_max_seg_size(&pdev->dev, UINT_MAX);
>>>>     drivers/mmc/host/renesas_sdhi_internal_dmac.c:338:
>>>>       dma_set_max_seg_size(dev, 0xffffffff);
>>>>     drivers/nvme/host/pci.c:2520:
>>>>       dma_set_max_seg_size(dev->dev, 0xffffffff);
>>>>
>>>> So this patch just casts the cur_len in the calculation to a
>>>> size_t type to fix the overflow issue, as it's not necessary
>>>> to change the type of cur_len after all.
>>>>
>>>> Fixes: 809eac54cdd6 ("iommu/dma: Implement scatterlist segment merging")
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com>
>>>
>>> Looks good to me, but I let Robin take a look too before I apply it,
>>> Robin?
>> I'll need to take a closer look at how exactly an overflow would happen here
> 
> It was triggered by a test case that was trying to map a 4GB
> dma_buf (1000+ nents in the scatterlist). This function then
> seemed to reduce the nents by merging most of them, probably
> because they were contiguous.

So, looking at the code now, the actual problem seems to be that this
overflow allows segments to be merged to or beyond 4GB total length,
thus sg->dma_length for the resulting segment also ends up wrapped and
truncated, and the caller will end up going downhill from there; is that
correct?

>> (just got back off some holiday), but my immediate thought is that if this
>> is a real problem, then what about 32-bit builds where size_t would still
>> overflow?
> 
> I think most of callers are also using size_t type for their
> size parameters like dma_buf, so the cur_len + s_length will
> unlikely go higher than UINT_MAX. But just in case that some
> driver allocates a large sg with a size parameter defined in
> 64-bit and uses this map() function, so it might be safer to
> change to "size_t" here to "u64"?

On reflection, I don't really think that size_t fits here anyway, since
all the members of the incoming struct scatterlist are unsigned int too.
Does the patch below work?

Robin.

----->8-----
From: Robin Murphy <robin.murphy@arm.com>
Subject: [PATCH] iommu/dma: Handle SG length overflow better

Since scatterlist dimensions are all unsigned ints, in the relatively
rare cases where a device's max_segment_size is set to UINT_MAX, then
the "cur_len + s_length <= max_len" check in __finalise_sg() will always
return true. As a result, the corner case of such a device mapping an
excessively large scatterlist which is mergeable to or beyond a total
length of 4GB can lead to overflow and a bogus truncated dma_length in
the resulting segment.

As we already assume that any single segment must be no longer than
max_len to begin with, this can easily be addressed by reshuffling the
comparison.

Fixes: 809eac54cdd6 ("iommu/dma: Implement scatterlist segment merging")
Reported-by: Nicolin Chen <nicoleotsuka@gmail.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/dma-iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index 129c4badf9ae..8de6cf623362 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -721,7 +721,7 @@ static int __finalise_sg(struct device *dev, struct scatterlist *sg, int nents,
 		 * - and wouldn't make the resulting output segment too long
 		 */
 		if (cur_len && !s_iova_off && (dma_addr & seg_mask) &&
-		    (cur_len + s_length <= max_len)) {
+		    (max_len - cur_len >= s_length)) {
 			/* ...then concatenate it with the previous one */
 			cur_len += s_length;
 		} else {

Re: [PATCH] iommu/dma: Fix calculation overflow in __finalise_sg()

[Nicolin Chen]

On Tue, Jul 02, 2019 at 11:40:02AM +0100, Robin Murphy wrote:
> On reflection, I don't really think that size_t fits here anyway, since
> all the members of the incoming struct scatterlist are unsigned int too.
> Does the patch below work?

Yes.

> ----->8-----
> From: Robin Murphy <robin.murphy@arm.com>
> Subject: [PATCH] iommu/dma: Handle SG length overflow better
> 
> Since scatterlist dimensions are all unsigned ints, in the relatively
> rare cases where a device's max_segment_size is set to UINT_MAX, then
> the "cur_len + s_length <= max_len" check in __finalise_sg() will always
> return true. As a result, the corner case of such a device mapping an
> excessively large scatterlist which is mergeable to or beyond a total
> length of 4GB can lead to overflow and a bogus truncated dma_length in
> the resulting segment.
> 
> As we already assume that any single segment must be no longer than
> max_len to begin with, this can easily be addressed by reshuffling the
> comparison.
> 
> Fixes: 809eac54cdd6 ("iommu/dma: Implement scatterlist segment merging")
> Reported-by: Nicolin Chen <nicoleotsuka@gmail.com>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

Tested-by: Nicolin Chen <nicoleotsuka@gmail.com>

Thank you!

> ---
>  drivers/iommu/dma-iommu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index 129c4badf9ae..8de6cf623362 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -721,7 +721,7 @@ static int __finalise_sg(struct device *dev, struct scatterlist *sg, int nents,
>  		 * - and wouldn't make the resulting output segment too long
>  		 */
>  		if (cur_len && !s_iova_off && (dma_addr & seg_mask) &&
> -		    (cur_len + s_length <= max_len)) {
> +		    (max_len - cur_len >= s_length)) {
>  			/* ...then concatenate it with the previous one */
>  			cur_len += s_length;
>  		} else {

Re: [PATCH] iommu/dma: Fix calculation overflow in __finalise_sg()

[Nicolin Chen]

Sorry to ping this but it's been a while.

Robin, did you get a chance to resend your version?

Thanks
Nicolin

On Tue, Jul 02, 2019 at 02:04:01PM -0700, Nicolin Chen wrote:
> On Tue, Jul 02, 2019 at 11:40:02AM +0100, Robin Murphy wrote:
> > On reflection, I don't really think that size_t fits here anyway, since
> > all the members of the incoming struct scatterlist are unsigned int too.
> > Does the patch below work?
> 
> Yes.
> 
> > ----->8-----
> > From: Robin Murphy <robin.murphy@arm.com>
> > Subject: [PATCH] iommu/dma: Handle SG length overflow better
> > 
> > Since scatterlist dimensions are all unsigned ints, in the relatively
> > rare cases where a device's max_segment_size is set to UINT_MAX, then
> > the "cur_len + s_length <= max_len" check in __finalise_sg() will always
> > return true. As a result, the corner case of such a device mapping an
> > excessively large scatterlist which is mergeable to or beyond a total
> > length of 4GB can lead to overflow and a bogus truncated dma_length in
> > the resulting segment.
> > 
> > As we already assume that any single segment must be no longer than
> > max_len to begin with, this can easily be addressed by reshuffling the
> > comparison.
> > 
> > Fixes: 809eac54cdd6 ("iommu/dma: Implement scatterlist segment merging")
> > Reported-by: Nicolin Chen <nicoleotsuka@gmail.com>
> > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> 
> Tested-by: Nicolin Chen <nicoleotsuka@gmail.com>
> 
> Thank you!
> 
> > ---
> >  drivers/iommu/dma-iommu.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> > index 129c4badf9ae..8de6cf623362 100644
> > --- a/drivers/iommu/dma-iommu.c
> > +++ b/drivers/iommu/dma-iommu.c
> > @@ -721,7 +721,7 @@ static int __finalise_sg(struct device *dev, struct scatterlist *sg, int nents,
> >  		 * - and wouldn't make the resulting output segment too long
> >  		 */
> >  		if (cur_len && !s_iova_off && (dma_addr & seg_mask) &&
> > -		    (cur_len + s_length <= max_len)) {
> > +		    (max_len - cur_len >= s_length)) {
> >  			/* ...then concatenate it with the previous one */
> >  			cur_len += s_length;
> >  		} else {