iommu/vt-d: Fix an off-by-one bug in __domain_mapping()

PATCH In review Jiang Liu 3 participants 5 messages Nov 26, 2014 – Dec 2, 2014 IOMMU

lore

Share & tools

lore

Jiang Liu

author

2014-11-26 01:38 UTC #0 lore raw

PATCH 1/1 iommu/vt-d: Fix an off-by-one bug in __domain_mapping()

There's an off-by-one bug in function __domain_mapping(), which may trigger the BUG_ON(nr_pages < lvl_pages) when

	(nr_pages + 1) & superpage_mask == 0

The issue was introduced by commit 9051aa0268dc "intel-iommu: Combine domain_pfn_mapping() and domain_sg_mapping()", which sets sg_res to "nr_pages + 1" to avoid some of the 'sg_res==0' code paths.

It's safe to remove extra "+1" because sg_res is only used to calculate page size now.

Reported-And-Tested-by Sudeep Dutt <[email protected]>

Signed-off-by Jiang Liu <[email protected]>

Cc <[email protected]> # 3.1

---
Hi David and Joerg,
This issue was introduced in v2.6.31, but intel-iommu.c has
been moved into drivers/iommu in v3.1. So what's the preferred way to deal with stable kernels between v2.6.31 and v3.1? Thanks!
Gerry

---
 drivers/iommu/intel-iommu.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

drivers/iommu/intel-iommu.c +3 -5

index a27d6cb1a793..b26ad10ec697 100644
@@ -1983,7 +1983,7 @@ static int __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn,
 {
 	struct dma_pte *first_pte = NULL, *pte = NULL;
 	phys_addr_t uninitialized_var(pteval);
-	unsigned long sg_res;
+	unsigned long sg_res = 0;
 	unsigned int largepage_lvl = 0;
 	unsigned long lvl_pages = 0;

@@ -1994,10 +1994,8 @@ static int __domain_mapping(struct dmar_domain *domain, unsigned long iov_pfn,
 
 	prot &= DMA_PTE_READ | DMA_PTE_WRITE | DMA_PTE_SNP;
 
-	if (sg)
-		sg_res = 0;
-	else {
-		sg_res = nr_pages + 1;
+	if (!sg) {
+		sg_res = nr_pages;
 		pteval = ((phys_addr_t)phys_pfn << VTD_PAGE_SHIFT) | prot;
 	}

Signature · 1 line

1.7.10.4

Joerg Roedel

maintainer

2014-12-01 16:27 UTC #1 lore raw

Quoted 3 lines · Jiang Liu

There's an off-by-one bug in function __domain_mapping(), which may trigger the BUG_ON(nr_pages < lvl_pages) when
	(nr_pages + 1) & superpage_mask == 0

What is the superpage_mask?

Quoted 8 lines

The issue was introduced by commit 9051aa0268dc "intel-iommu: Combine domain_pfn_mapping() and domain_sg_mapping()", which sets sg_res to "nr_pages + 1" to avoid some of the 'sg_res==0' code paths.
It's safe to remove extra "+1" because sg_res is only used to calculate page size now.
From your description and the (hard to read) code in __domain_mapping I

don't really understand the issue yet. Can you please elaborate on this issue can be triggered?

Is the BUG_ON the only issue and, if yes, can that be fixed by just changing the BUG_ON condition?

Quoted 3 lines

This issue was introduced in v2.6.31, but intel-iommu.c has
been moved into drivers/iommu in v3.1. So what's the preferred way to deal with stable kernels between v2.6.31 and v3.1?

Just remove the kernel version marker from the stable tag. The stable kernel maintainers for kernels >3.1 will ask you to backport the patch or just backport it by themselfes.

Joerg

Jiang Liu

author

2014-12-02 00:06 UTC #2 lore raw

Quoted 6 lines · Joerg Roedel

Quoted 3 lines · Jiang Liu
There's an off-by-one bug in function __domain_mapping(), which may trigger the BUG_ON(nr_pages < lvl_pages) when
	(nr_pages + 1) & superpage_mask == 0
What is the superpage_mask?

Hi Joerg,
Sorry for the confusion. The really story is:
1) sg_res is set to nr_pages + 1 at the beginning of __domain_mapping()
2) then sg_res is used to choose super page by function hardware_largepage_caps(domain, iov_pfn, phys_pfn, sg_res).
The condition to trigger the issue is:
__domain_mapping is called by domain_pfn_mapping() with nr_pages of 511, so sg_res is 512 and hardware_largepage_caps() will choose a wrong super page size of 2M, which then trigger BUG_ON(sg_res < lvl_pages).

So it's not only a BUG_ON() issue, but also causes incorrect super page selection.
Thanks!
Gerry

Quoted 26 lines

Quoted 7 lines
The issue was introduced by commit 9051aa0268dc "intel-iommu: Combine domain_pfn_mapping() and domain_sg_mapping()", which sets sg_res to "nr_pages + 1" to avoid some of the 'sg_res==0' code paths.
It's safe to remove extra "+1" because sg_res is only used to calculate page size now.
From your description and the (hard to read) code in __domain_mapping I don't really understand the issue yet. Can you please elaborate on this issue can be triggered?
Is the BUG_ON the only issue and, if yes, can that be fixed by just changing the BUG_ON condition?
Quoted 3 lines
This issue was introduced in v2.6.31, but intel-iommu.c has
been moved into drivers/iommu in v3.1. So what's the preferred way to deal with stable kernels between v2.6.31 and v3.1?
Just remove the kernel version marker from the stable tag. The stable kernel maintainers for kernels >3.1 will ask you to backport the patch or just backport it by themselfes.
Joerg

David Woodhouse

Acked-by

2014-12-02 10:34 UTC #3 lore raw

In reply to Joerg Roedel

Quoted 14 lines · Joerg Roedel

Quoted 3 lines · Jiang Liu
There's an off-by-one bug in function __domain_mapping(), which may trigger the BUG_ON(nr_pages < lvl_pages) when
	(nr_pages + 1) & superpage_mask == 0
What is the superpage_mask?
Quoted 6 lines
The issue was introduced by commit 9051aa0268dc "intel-iommu: Combine domain_pfn_mapping() and domain_sg_mapping()", which sets sg_res to "nr_pages + 1" to avoid some of the 'sg_res==0' code paths.
It's safe to remove extra "+1" because sg_res is only used to calculate page size now.

Quoted 6 lines

From your description and the (hard to read) code in __domain_mapping I don't really understand the issue yet. Can you please elaborate on this issue can be triggered?
Is the BUG_ON the only issue and, if yes, can that be fixed by just changing the BUG_ON condition?

__domain_mapping() is an amalgamation of the old domain_pfn_mapping() and domain_sg_mapping() functions. When I did that, in commit 9051aa026, the 'sg_res' variable was used *only* for tracking how many pages were left in the current scatterlist element, before we had to get the next one from the sglist.

For reasons which are lost now, in the case of a simple pfn range I was setting 'sg_res = nr_pages + 1' to ensure that we *never* got down to sg_res=0 and tried to look for more from the (non-existent, in this case) sglist.

Later in commit 6dd9a7c73 we added large page support, using sg_res in a way which actually required it to be accurate. And now we have an off-by-one because we'll actually *try* to use a 2GiB large page for a mapping of size 0x1ff000, because of that '+1'.

The BUG_ON is entirely correct here, and correctly highlighted the problem.

However, the +1 is no longer necessary, because the check that needed it was also modified to read 'if (sg_res && nr_pages)', which is perfectly sufficient and arguably how it should have been done in the first place.

I had an almost identical patch last week for internal testing, because I stupidly hadn't noticed that Jiang had beaten me to it.

Acked-by David Woodhouse <[email protected]>

Quoted 7 lines

Quoted 3 lines
This issue was introduced in v2.6.31, but intel-iommu.c has
been moved into drivers/iommu in v3.1. So what's the preferred way to deal with stable kernels between v2.6.31 and v3.1?
Just remove the kernel version marker from the stable tag. The stable kernel maintainers for kernels >3.1 will ask you to backport the patch or just backport it by themselfes.

I think this is only an issue since commit 6dd9a7c737 added super page support in 3.0, isn't it? Before that, the +1 was *needed*.

Signature · 1 line

dwmw2

Joerg Roedel

maintainer

2014-12-02 12:04 UTC #4 lore raw

Quoted 32 lines · David Woodhouse

__domain_mapping() is an amalgamation of the old domain_pfn_mapping() and domain_sg_mapping() functions. When I did that, in commit 9051aa026, the 'sg_res' variable was used *only* for tracking how many pages were left in the current scatterlist element, before we had to get the next one from the sglist.
For reasons which are lost now, in the case of a simple pfn range I was setting 'sg_res = nr_pages + 1' to ensure that we *never* got down to sg_res=0 and tried to look for more from the (non-existent, in this case) sglist.
Later in commit 6dd9a7c73 we added large page support, using sg_res in a way which actually required it to be accurate. And now we have an off-by-one because we'll actually *try* to use a 2GiB large page for a mapping of size 0x1ff000, because of that '+1'.
The BUG_ON is entirely correct here, and correctly highlighted the problem.
However, the +1 is no longer necessary, because the check that needed it was also modified to read 'if (sg_res && nr_pages)', which is perfectly sufficient and arguably how it should have been done in the first place.
I had an almost identical patch last week for internal testing, because I stupidly hadn't noticed that Jiang had beaten me to it.
Acked-by David Woodhouse <[email protected]>
Quoted 4 lines
Quoted 3 lines
This issue was introduced in v2.6.31, but intel-iommu.c has
been moved into drivers/iommu in v3.1. So what's the preferred way to deal with stable kernels between v2.6.31 and v3.1?

Quoted 6 lines

Quoted 3 lines
Just remove the kernel version marker from the stable tag. The stable kernel maintainers for kernels >3.1 will ask you to backport the patch or just backport it by themselfes.
I think this is only an issue since commit 6dd9a7c737 added super page support in 3.0, isn't it? Before that, the +1 was *needed*.

Okay guys, thanks for the explanations. I applied the patch to the x86/vt-d branch and changed the stable tag to >= 3.0.

Joerg