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

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

[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

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 <sudeep.dutt@intel.com>
Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
Cc: <stable@vger.kernel.org> # 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(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index a27d6cb1a793..b26ad10ec697 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -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;
 	}
 
-- 
1.7.10.4

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

[Joerg Roedel]

On Wed, Nov 26, 2014 at 09:42:10AM +0800, Jiang Liu wrote:
> 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?

> 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?

> 	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

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

[Jiang Liu]

On 2014/12/2 0:27, Joerg Roedel wrote:
> On Wed, Nov 26, 2014 at 09:42:10AM +0800, Jiang Liu wrote:
>> 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

> 
>> 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?
> 
>> 	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
> 

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

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 2766 bytes --]

On Mon, 2014-12-01 at 17:27 +0100, Joerg Roedel wrote:
> On Wed, Nov 26, 2014 at 09:42:10AM +0800, Jiang Liu wrote:
> > 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?
> 
> > 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?

__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 <David.Woodhouse@intel.com>

> > 	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*.

-- 
dwmw2

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5745 bytes --]

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

[Joerg Roedel]

On Tue, Dec 02, 2014 at 10:34:35AM +0000, David Woodhouse wrote:
> __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 <David.Woodhouse@intel.com>
> 
> > > 	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*.

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