[Trusty SRU: CVE-2017-12134] xen: fix bio vec merging

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[Trusty SRU: CVE-2017-12134] xen: fix bio vec merging

Stefan Bader-2
From: Roger Pau Monne <[hidden email]>

The current test for bio vec merging is not fully accurate and can be
tricked into merging bios when certain grant combinations are used.
The result of these malicious bio merges is a bio that extends past
the memory page used by any of the originating bios.

Take into account the following scenario, where a guest creates two
grant references that point to the same mfn, ie: grant 1 -> mfn A,
grant 2 -> mfn A.

These references are then used in a PV block request, and mapped by
the backend domain, thus obtaining two different pfns that point to
the same mfn, pfn B -> mfn A, pfn C -> mfn A.

If those grants happen to be used in two consecutive sectors of a disk
IO operation becoming two different bios in the backend domain, the
checks in xen_biovec_phys_mergeable will succeed, because bfn1 == bfn2
(they both point to the same mfn). However due to the bio merging,
the backend domain will end up with a bio that expands past mfn A into
mfn A + 1.

Fix this by making sure the check in xen_biovec_phys_mergeable takes
into account the offset and the length of the bio, this basically
replicates whats done in __BIOVEC_PHYS_MERGEABLE using mfns (bus
addresses). While there also remove the usage of
__BIOVEC_PHYS_MERGEABLE, since that's already checked by the callers
of xen_biovec_phys_mergeable.

CC: [hidden email]
Reported-by: "Jan H. Schönherr" <[hidden email]>
Signed-off-by: Roger Pau Monné <[hidden email]>
Reviewed-by: Juergen Gross <[hidden email]>
Signed-off-by: Konrad Rzeszutek Wilk <[hidden email]>

CVE-2017-12134

(backported from commit 462cdace790ac2ed6aad1b19c9c0af0143b6aab0)
[smb: pfn_to_gfn in newer code returns the same as pfn_to_mfn]
Signed-off-by: Stefan Bader <[hidden email]>
---
Already applied in Xenial, Trusty still needs it but needed a bit
of tweaking. Basically the newer code uses pfn_to_gfn which either
returns a pfn or a mfn. Which is the same behavior as pfn_to_mfn.

-Stefan

 drivers/xen/biomerge.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/xen/biomerge.c b/drivers/xen/biomerge.c
index 0edb91c..4a77fe8 100644
--- a/drivers/xen/biomerge.c
+++ b/drivers/xen/biomerge.c
@@ -9,7 +9,6 @@ bool xen_biovec_phys_mergeable(const struct bio_vec *vec1,
  unsigned long mfn1 = pfn_to_mfn(page_to_pfn(vec1->bv_page));
  unsigned long mfn2 = pfn_to_mfn(page_to_pfn(vec2->bv_page));
 
- return __BIOVEC_PHYS_MERGEABLE(vec1, vec2) &&
- ((mfn1 == mfn2) || ((mfn1+1) == mfn2));
+ return mfn1 + PFN_DOWN(vec1->bv_offset + vec1->bv_len) == mfn2;
 }
 EXPORT_SYMBOL(xen_biovec_phys_mergeable);
--
2.7.4


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [Trusty SRU: CVE-2017-12134] xen: fix bio vec merging

Kleber Sacilotto de Souza
On 04/17/18 11:46, Stefan Bader wrote:

> From: Roger Pau Monne <[hidden email]>
>
> The current test for bio vec merging is not fully accurate and can be
> tricked into merging bios when certain grant combinations are used.
> The result of these malicious bio merges is a bio that extends past
> the memory page used by any of the originating bios.
>
> Take into account the following scenario, where a guest creates two
> grant references that point to the same mfn, ie: grant 1 -> mfn A,
> grant 2 -> mfn A.
>
> These references are then used in a PV block request, and mapped by
> the backend domain, thus obtaining two different pfns that point to
> the same mfn, pfn B -> mfn A, pfn C -> mfn A.
>
> If those grants happen to be used in two consecutive sectors of a disk
> IO operation becoming two different bios in the backend domain, the
> checks in xen_biovec_phys_mergeable will succeed, because bfn1 == bfn2
> (they both point to the same mfn). However due to the bio merging,
> the backend domain will end up with a bio that expands past mfn A into
> mfn A + 1.
>
> Fix this by making sure the check in xen_biovec_phys_mergeable takes
> into account the offset and the length of the bio, this basically
> replicates whats done in __BIOVEC_PHYS_MERGEABLE using mfns (bus
> addresses). While there also remove the usage of
> __BIOVEC_PHYS_MERGEABLE, since that's already checked by the callers
> of xen_biovec_phys_mergeable.
>
> CC: [hidden email]
> Reported-by: "Jan H. Schönherr" <[hidden email]>
> Signed-off-by: Roger Pau Monné <[hidden email]>
> Reviewed-by: Juergen Gross <[hidden email]>
> Signed-off-by: Konrad Rzeszutek Wilk <[hidden email]>
>
> CVE-2017-12134
>
> (backported from commit 462cdace790ac2ed6aad1b19c9c0af0143b6aab0)
> [smb: pfn_to_gfn in newer code returns the same as pfn_to_mfn]
> Signed-off-by: Stefan Bader <[hidden email]>
> ---
> Already applied in Xenial, Trusty still needs it but needed a bit
> of tweaking. Basically the newer code uses pfn_to_gfn which either
> returns a pfn or a mfn. Which is the same behavior as pfn_to_mfn.
>
> -Stefan

Acked-by: Kleber Sacilotto de Souza <[hidden email]>

>
>  drivers/xen/biomerge.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/xen/biomerge.c b/drivers/xen/biomerge.c
> index 0edb91c..4a77fe8 100644
> --- a/drivers/xen/biomerge.c
> +++ b/drivers/xen/biomerge.c
> @@ -9,7 +9,6 @@ bool xen_biovec_phys_mergeable(const struct bio_vec *vec1,
>   unsigned long mfn1 = pfn_to_mfn(page_to_pfn(vec1->bv_page));
>   unsigned long mfn2 = pfn_to_mfn(page_to_pfn(vec2->bv_page));
>  
> - return __BIOVEC_PHYS_MERGEABLE(vec1, vec2) &&
> - ((mfn1 == mfn2) || ((mfn1+1) == mfn2));
> + return mfn1 + PFN_DOWN(vec1->bv_offset + vec1->bv_len) == mfn2;
>  }
>  EXPORT_SYMBOL(xen_biovec_phys_mergeable);
>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [Trusty SRU: CVE-2017-12134] xen: fix bio vec merging

Colin Ian King-2
In reply to this post by Stefan Bader-2
On 17/04/18 10:46, Stefan Bader wrote:

> From: Roger Pau Monne <[hidden email]>
>
> The current test for bio vec merging is not fully accurate and can be
> tricked into merging bios when certain grant combinations are used.
> The result of these malicious bio merges is a bio that extends past
> the memory page used by any of the originating bios.
>
> Take into account the following scenario, where a guest creates two
> grant references that point to the same mfn, ie: grant 1 -> mfn A,
> grant 2 -> mfn A.
>
> These references are then used in a PV block request, and mapped by
> the backend domain, thus obtaining two different pfns that point to
> the same mfn, pfn B -> mfn A, pfn C -> mfn A.
>
> If those grants happen to be used in two consecutive sectors of a disk
> IO operation becoming two different bios in the backend domain, the
> checks in xen_biovec_phys_mergeable will succeed, because bfn1 == bfn2
> (they both point to the same mfn). However due to the bio merging,
> the backend domain will end up with a bio that expands past mfn A into
> mfn A + 1.
>
> Fix this by making sure the check in xen_biovec_phys_mergeable takes
> into account the offset and the length of the bio, this basically
> replicates whats done in __BIOVEC_PHYS_MERGEABLE using mfns (bus
> addresses). While there also remove the usage of
> __BIOVEC_PHYS_MERGEABLE, since that's already checked by the callers
> of xen_biovec_phys_mergeable.
>
> CC: [hidden email]
> Reported-by: "Jan H. Schönherr" <[hidden email]>
> Signed-off-by: Roger Pau Monné <[hidden email]>
> Reviewed-by: Juergen Gross <[hidden email]>
> Signed-off-by: Konrad Rzeszutek Wilk <[hidden email]>
>
> CVE-2017-12134
>
> (backported from commit 462cdace790ac2ed6aad1b19c9c0af0143b6aab0)
> [smb: pfn_to_gfn in newer code returns the same as pfn_to_mfn]
> Signed-off-by: Stefan Bader <[hidden email]>
> ---
> Already applied in Xenial, Trusty still needs it but needed a bit
> of tweaking. Basically the newer code uses pfn_to_gfn which either
> returns a pfn or a mfn. Which is the same behavior as pfn_to_mfn.
>
> -Stefan
>
>  drivers/xen/biomerge.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/xen/biomerge.c b/drivers/xen/biomerge.c
> index 0edb91c..4a77fe8 100644
> --- a/drivers/xen/biomerge.c
> +++ b/drivers/xen/biomerge.c
> @@ -9,7 +9,6 @@ bool xen_biovec_phys_mergeable(const struct bio_vec *vec1,
>   unsigned long mfn1 = pfn_to_mfn(page_to_pfn(vec1->bv_page));
>   unsigned long mfn2 = pfn_to_mfn(page_to_pfn(vec2->bv_page));
>  
> - return __BIOVEC_PHYS_MERGEABLE(vec1, vec2) &&
> - ((mfn1 == mfn2) || ((mfn1+1) == mfn2));
> + return mfn1 + PFN_DOWN(vec1->bv_offset + vec1->bv_len) == mfn2;
>  }
>  EXPORT_SYMBOL(xen_biovec_phys_mergeable);
>

After double checking this more than once I'm confident this backport is
correct.

Acked-by: Colin Ian King <[hidden email]>

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED: [Trusty SRU: CVE-2017-12134] xen: fix bio vec merging

Stefan Bader-2
In reply to this post by Stefan Bader-2
On 17.04.2018 11:46, Stefan Bader wrote:

> From: Roger Pau Monne <[hidden email]>
>
> The current test for bio vec merging is not fully accurate and can be
> tricked into merging bios when certain grant combinations are used.
> The result of these malicious bio merges is a bio that extends past
> the memory page used by any of the originating bios.
>
> Take into account the following scenario, where a guest creates two
> grant references that point to the same mfn, ie: grant 1 -> mfn A,
> grant 2 -> mfn A.
>
> These references are then used in a PV block request, and mapped by
> the backend domain, thus obtaining two different pfns that point to
> the same mfn, pfn B -> mfn A, pfn C -> mfn A.
>
> If those grants happen to be used in two consecutive sectors of a disk
> IO operation becoming two different bios in the backend domain, the
> checks in xen_biovec_phys_mergeable will succeed, because bfn1 == bfn2
> (they both point to the same mfn). However due to the bio merging,
> the backend domain will end up with a bio that expands past mfn A into
> mfn A + 1.
>
> Fix this by making sure the check in xen_biovec_phys_mergeable takes
> into account the offset and the length of the bio, this basically
> replicates whats done in __BIOVEC_PHYS_MERGEABLE using mfns (bus
> addresses). While there also remove the usage of
> __BIOVEC_PHYS_MERGEABLE, since that's already checked by the callers
> of xen_biovec_phys_mergeable.
>
> CC: [hidden email]
> Reported-by: "Jan H. Schönherr" <[hidden email]>
> Signed-off-by: Roger Pau Monné <[hidden email]>
> Reviewed-by: Juergen Gross <[hidden email]>
> Signed-off-by: Konrad Rzeszutek Wilk <[hidden email]>
>
> CVE-2017-12134
>
> (backported from commit 462cdace790ac2ed6aad1b19c9c0af0143b6aab0)
> [smb: pfn_to_gfn in newer code returns the same as pfn_to_mfn]
> Signed-off-by: Stefan Bader <[hidden email]>
> ---
Applied to trusty/master-next

> Already applied in Xenial, Trusty still needs it but needed a bit
> of tweaking. Basically the newer code uses pfn_to_gfn which either
> returns a pfn or a mfn. Which is the same behavior as pfn_to_mfn.
>
> -Stefan
>
>  drivers/xen/biomerge.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/xen/biomerge.c b/drivers/xen/biomerge.c
> index 0edb91c..4a77fe8 100644
> --- a/drivers/xen/biomerge.c
> +++ b/drivers/xen/biomerge.c
> @@ -9,7 +9,6 @@ bool xen_biovec_phys_mergeable(const struct bio_vec *vec1,
>   unsigned long mfn1 = pfn_to_mfn(page_to_pfn(vec1->bv_page));
>   unsigned long mfn2 = pfn_to_mfn(page_to_pfn(vec2->bv_page));
>  
> - return __BIOVEC_PHYS_MERGEABLE(vec1, vec2) &&
> - ((mfn1 == mfn2) || ((mfn1+1) == mfn2));
> + return mfn1 + PFN_DOWN(vec1->bv_offset + vec1->bv_len) == mfn2;
>  }
>  EXPORT_SYMBOL(xen_biovec_phys_mergeable);
>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (836 bytes) Download Attachment