| Message ID | 1523958412-25466-1-git-send-email-stefan.bader@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [Trusty,SRU:,CVE-2017-12134] xen: fix bio vec merging | expand |
On 04/17/18 11:46, Stefan Bader wrote: > From: Roger Pau Monne <roger.pau@citrix.com> > > 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: stable@vger.kernel.org > Reported-by: "Jan H. Schönherr" <jschoenh@amazon.de> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > Reviewed-by: Juergen Gross <jgross@suse.com> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > 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 <stefan.bader@canonical.com> > --- > 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 <kleber.souza@canonical.com> > > 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); >
On 17/04/18 10:46, Stefan Bader wrote: > From: Roger Pau Monne <roger.pau@citrix.com> > > 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: stable@vger.kernel.org > Reported-by: "Jan H. Schönherr" <jschoenh@amazon.de> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > Reviewed-by: Juergen Gross <jgross@suse.com> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > 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 <stefan.bader@canonical.com> > --- > 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 <colin.king@canonical.com>
On 17.04.2018 11:46, Stefan Bader wrote: > From: Roger Pau Monne <roger.pau@citrix.com> > > 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: stable@vger.kernel.org > Reported-by: "Jan H. Schönherr" <jschoenh@amazon.de> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > Reviewed-by: Juergen Gross <jgross@suse.com> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > > 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 <stefan.bader@canonical.com> > --- 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); >
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);