i386/xen: prevent guest from binding loopback event channel to itself

Posted by David Woodhouse 9 months, 1 week ago

From: David Woodhouse <dwmw@amazon.co.uk>

Fuzzing showed that a guest could bind an interdomain port to itself, by
guessing the next port to be allocated and putting that as the 'remote'
port number. By chance, that works because the newly-allocated port has
type EVTCHNSTAT_unbound. It shouldn't.

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---
 hw/i386/kvm/xen_evtchn.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/hw/i386/kvm/xen_evtchn.c b/hw/i386/kvm/xen_evtchn.c
index 0e9c108614..70b4b8a6ef 100644
--- a/hw/i386/kvm/xen_evtchn.c
+++ b/hw/i386/kvm/xen_evtchn.c
@@ -1408,8 +1408,15 @@ int xen_evtchn_bind_interdomain_op(struct evtchn_bind_interdomain *interdomain)
         XenEvtchnPort *rp = &s->port_table[interdomain->remote_port];
         XenEvtchnPort *lp = &s->port_table[interdomain->local_port];
 
-        if (rp->type == EVTCHNSTAT_unbound && rp->type_val == 0) {
-            /* It's a match! */
+        /*
+         * The 'remote' port for loopback must be an unbound port allocated for
+         * communication with the local domain (as indicated by rp->type_val
+         * being zero, not PORT_INFO_TYPEVAL_REMOTE_QEMU), and must *not* be
+         * the port that was just allocated for the local end.
+         */
+        if (interdomain->local_port != interdomain->remote_port &&
+            rp->type == EVTCHNSTAT_unbound && rp->type_val == 0) {
+
             rp->type = EVTCHNSTAT_interdomain;
             rp->type_val = interdomain->local_port;
 
-- 
2.34.1

Re: i386/xen: prevent guest from binding loopback event channel to itself

Posted by Paul Durrant 9 months, 1 week ago

On 25/07/2023 11:05, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> Fuzzing showed that a guest could bind an interdomain port to itself, by
> guessing the next port to be allocated and putting that as the 'remote'
> port number. By chance, that works because the newly-allocated port has
> type EVTCHNSTAT_unbound. It shouldn't.
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
>   hw/i386/kvm/xen_evtchn.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 

Reviewed-by: Paul Durrant <paul@xen.org>

Re: i386/xen: prevent guest from binding loopback event channel to itself

Posted by David Woodhouse 9 months, 1 week ago

On Wed, 2023-07-26 at 09:44 +0100, Paul Durrant wrote:
> On 25/07/2023 11:05, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > Fuzzing showed that a guest could bind an interdomain port to itself, by
> > guessing the next port to be allocated and putting that as the 'remote'
> > port number. By chance, that works because the newly-allocated port has
> > type EVTCHNSTAT_unbound. It shouldn't.
> > 
> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> > ---
> >   hw/i386/kvm/xen_evtchn.c | 11 +++++++++--
> >   1 file changed, 9 insertions(+), 2 deletions(-)
> > 
> 
> Reviewed-by: Paul Durrant <paul@xen.org>
> 

Thanks. I'll change the title prefix to 'hw/xen' since it's in hw/ not
target/i386. Please can I have also have a review for
https://lore.kernel.org/qemu-devel/20076888f6bdf06a65aafc5cf954260965d45b97.camel@infradead.org/

I'll then send these outstanding patches from my tree as a series for
8.1:

David Woodhouse (4):
      hw/xen: Clarify (lack of) error handling in transaction_commit()
      hw/xen: fix off-by-one in xen_evtchn_set_gsi()
      i386/xen: consistent locking around Xen singleshot timers
      hw/xen: prevent guest from binding loopback event channel to itself

Re: i386/xen: prevent guest from binding loopback event channel to itself

Posted by Paul Durrant 9 months, 1 week ago

On 26/07/2023 10:07, David Woodhouse wrote:
> On Wed, 2023-07-26 at 09:44 +0100, Paul Durrant wrote:
>> On 25/07/2023 11:05, David Woodhouse wrote:
>>> From: David Woodhouse <dwmw@amazon.co.uk>
>>>
>>> Fuzzing showed that a guest could bind an interdomain port to itself, by
>>> guessing the next port to be allocated and putting that as the 'remote'
>>> port number. By chance, that works because the newly-allocated port has
>>> type EVTCHNSTAT_unbound. It shouldn't.
>>>
>>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
>>> ---
>>>    hw/i386/kvm/xen_evtchn.c | 11 +++++++++--
>>>    1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>
>> Reviewed-by: Paul Durrant <paul@xen.org>
>>
> 
> Thanks. I'll change the title prefix to 'hw/xen' since it's in hw/ not
> target/i386.

Yes, makes sense.

> Please can I have also have a review for
> https://lore.kernel.org/qemu-devel/20076888f6bdf06a65aafc5cf954260965d45b97.camel@infradead.org/
>

Sorry I missed that. Done.

Cheers,

   Paul

Re: i386/xen: prevent guest from binding loopback event channel to itself

Posted by Bernhard Beschow 9 months, 1 week ago


Am 26. Juli 2023 09:24:28 UTC schrieb Paul Durrant <xadimgnik@gmail.com>:
>On 26/07/2023 10:07, David Woodhouse wrote:
>> On Wed, 2023-07-26 at 09:44 +0100, Paul Durrant wrote:
>>> On 25/07/2023 11:05, David Woodhouse wrote:
>>>> From: David Woodhouse <dwmw@amazon.co.uk>
>>>> 
>>>> Fuzzing showed that a guest could bind an interdomain port to itself, by
>>>> guessing the next port to be allocated and putting that as the 'remote'
>>>> port number. By chance, that works because the newly-allocated port has
>>>> type EVTCHNSTAT_unbound. It shouldn't.
>>>> 
>>>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
>>>> ---
>>>>    hw/i386/kvm/xen_evtchn.c | 11 +++++++++--
>>>>    1 file changed, 9 insertions(+), 2 deletions(-)
>>>> 
>>> 
>>> Reviewed-by: Paul Durrant <paul@xen.org>
>>> 
>> 
>> Thanks. I'll change the title prefix to 'hw/xen' since it's in hw/ not
>> target/i386.
>
>Yes, makes sense.
>
>> Please can I have also have a review for
>> https://lore.kernel.org/qemu-devel/20076888f6bdf06a65aafc5cf954260965d45b97.camel@infradead.org/
>> 
>
>Sorry I missed that. Done.

And that one, too please? https://lore.kernel.org/qemu-devel/20230720072950.20198-1-olaf@aepfle.de/

Sorry for cross posting, but the patch would be good to have in 8.1.

Best regards,
Bernhard

>
>Cheers,
>
>  Paul
>