| Message ID | 20201218141811.310267-2-imbrenda@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/4] s390/kvm: VSIE: stop leaking host addresses | expand |
On 18.12.20 15:18, Claudio Imbrenda wrote: > The addresses in the SIE control block of the host should not be > forwarded to the guest. They are only meaningful to the host, and > moreover it would be a clear security issue. It's really almost impossible for someone without access to documentation to understand what we leak. I assume we're leaking the g1 address of a page table (entry), used for translation of g2->g3 to g1. Can you try making that clearer? In that case, it's pretty much a random number (of a random page used as a leave page table) and does not let g1 identify locations of symbols etc. If so, I don't think this is a "clear security issue" and suggest squashing this into the actual fix (#p4 I assume). @Christian, @Janosch? Am I missing something? > > Subsequent patches will actually put the right values in the guest SIE > control block. > > Fixes: a3508fbe9dc6d ("KVM: s390: vsie: initial support for nested virtualization") > Cc: stable@vger.kernel.org > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > --- > arch/s390/kvm/vsie.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c > index 4f3cbf6003a9..ada49583e530 100644 > --- a/arch/s390/kvm/vsie.c > +++ b/arch/s390/kvm/vsie.c > @@ -416,11 +416,6 @@ static void unshadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) > memcpy((void *)((u64)scb_o + 0xc0), > (void *)((u64)scb_s + 0xc0), 0xf0 - 0xc0); > break; > - case ICPT_PARTEXEC: > - /* MVPG only */ > - memcpy((void *)((u64)scb_o + 0xc0), > - (void *)((u64)scb_s + 0xc0), 0xd0 - 0xc0); > - break; > } > > if (scb_s->ihcpu != 0xffffU) > -- Thanks, David / dhildenb
On Sun, 20 Dec 2020 10:44:56 +0100 David Hildenbrand <david@redhat.com> wrote: > On 18.12.20 15:18, Claudio Imbrenda wrote: > > The addresses in the SIE control block of the host should not be > > forwarded to the guest. They are only meaningful to the host, and > > moreover it would be a clear security issue. > > It's really almost impossible for someone without access to > documentation to understand what we leak. I assume we're leaking the > g1 address of a page table (entry), used for translation of g2->g3 to > g1. Can you try making that clearer? this is correct. I guess I can improve the text of the commit > In that case, it's pretty much a random number (of a random page used > as a leave page table) and does not let g1 identify locations of > symbols etc. If so, I don't think this is a "clear security issue" > and suggest squashing this into the actual fix (#p4 I assume). yeah __maybe__ I overstated the importance ;) But I would still like to keep it as a separate patch, looks more straightforward to me > @Christian, @Janosch? Am I missing something? > > > > > Subsequent patches will actually put the right values in the guest > > SIE control block. > > > > Fixes: a3508fbe9dc6d ("KVM: s390: vsie: initial support for nested > > virtualization") Cc: stable@vger.kernel.org > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > > --- > > arch/s390/kvm/vsie.c | 5 ----- > > 1 file changed, 5 deletions(-) > > > > diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c > > index 4f3cbf6003a9..ada49583e530 100644 > > --- a/arch/s390/kvm/vsie.c > > +++ b/arch/s390/kvm/vsie.c > > @@ -416,11 +416,6 @@ static void unshadow_scb(struct kvm_vcpu > > *vcpu, struct vsie_page *vsie_page) memcpy((void *)((u64)scb_o + > > 0xc0), (void *)((u64)scb_s + 0xc0), 0xf0 - 0xc0); > > break; > > - case ICPT_PARTEXEC: > > - /* MVPG only */ > > - memcpy((void *)((u64)scb_o + 0xc0), > > - (void *)((u64)scb_s + 0xc0), 0xd0 - 0xc0); > > - break; > > } > > > > if (scb_s->ihcpu != 0xffffU) > > > >
>> In that case, it's pretty much a random number (of a random page used >> as a leave page table) and does not let g1 identify locations of >> symbols etc. If so, I don't think this is a "clear security issue" >> and suggest squashing this into the actual fix (#p4 I assume). > > yeah __maybe__ I overstated the importance ;) > > But I would still like to keep it as a separate patch, looks more > straightforward to me > I don't see a need to split this up. Just fix it right away. -- Thanks, David / dhildenb
On 12/18/20 3:18 PM, Claudio Imbrenda wrote: > The addresses in the SIE control block of the host should not be > forwarded to the guest. They are only meaningful to the host, and > moreover it would be a clear security issue. > > Subsequent patches will actually put the right values in the guest SIE > control block. > > Fixes: a3508fbe9dc6d ("KVM: s390: vsie: initial support for nested virtualization") > Cc: stable@vger.kernel.org > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> Looks reasonable. Give me some time to have a deeper look it's a lot of architecture to read. > --- > arch/s390/kvm/vsie.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c > index 4f3cbf6003a9..ada49583e530 100644 > --- a/arch/s390/kvm/vsie.c > +++ b/arch/s390/kvm/vsie.c > @@ -416,11 +416,6 @@ static void unshadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) > memcpy((void *)((u64)scb_o + 0xc0), > (void *)((u64)scb_s + 0xc0), 0xf0 - 0xc0); > break; > - case ICPT_PARTEXEC: > - /* MVPG only */ > - memcpy((void *)((u64)scb_o + 0xc0), > - (void *)((u64)scb_s + 0xc0), 0xd0 - 0xc0); > - break; > } > > if (scb_s->ihcpu != 0xffffU) >
diff --git a/arch/s390/kvm/vsie.c b/arch/s390/kvm/vsie.c index 4f3cbf6003a9..ada49583e530 100644 --- a/arch/s390/kvm/vsie.c +++ b/arch/s390/kvm/vsie.c @@ -416,11 +416,6 @@ static void unshadow_scb(struct kvm_vcpu *vcpu, struct vsie_page *vsie_page) memcpy((void *)((u64)scb_o + 0xc0), (void *)((u64)scb_s + 0xc0), 0xf0 - 0xc0); break; - case ICPT_PARTEXEC: - /* MVPG only */ - memcpy((void *)((u64)scb_o + 0xc0), - (void *)((u64)scb_s + 0xc0), 0xd0 - 0xc0); - break; } if (scb_s->ihcpu != 0xffffU)
The addresses in the SIE control block of the host should not be forwarded to the guest. They are only meaningful to the host, and moreover it would be a clear security issue. Subsequent patches will actually put the right values in the guest SIE control block. Fixes: a3508fbe9dc6d ("KVM: s390: vsie: initial support for nested virtualization") Cc: stable@vger.kernel.org Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> --- arch/s390/kvm/vsie.c | 5 ----- 1 file changed, 5 deletions(-)