netdev - Re: [PATCH] VSOCK: bind to random port for VMADDR_PORT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 11 Dec 2018 18:53:06 +0000
From:   "Jorgen S. Hansen" <jhansen@...are.com>
To:     Stefan Hajnoczi <stefanha@...hat.com>,
        Dexuan Cui <decui@...rosoft.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Lepton Wu <ytht.net@...il.com>
Subject: Re: [PATCH] VSOCK: bind to random port for VMADDR_PORT_ANY


> On Mon, Dec 10, 2018 at 11:02:35PM -0800, Lepton Wu wrote:
> > The old code always starts from fixed port for VMADDR_PORT_ANY. Sometimes
> > when VMM crashed, there is still orphaned vsock which is waiting for
> > close timer, then it could cause connection time out for new started VM
> > if they are trying to connect to same port with same guest cid since the
> > new packets could hit that orphaned vsock. We could also fix this by doing
> > more in vhost_vsock_reset_orphans, but any way, it should be better to start
> > from a random local port instead of a fixed one.
> >
> > Signed-off-by: Lepton Wu <ytht.net@...il.com>
> > ---
> >  net/vmw_vsock/af_vsock.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
>
> Jorgen, Dexuan: Any objection to this?  It also affects the other
> AF_VSOCK transports.

Makes sense to me.

> > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> > index ab27a2872935..73817e846a1f 100644
> > --- a/net/vmw_vsock/af_vsock.c
> > +++ b/net/vmw_vsock/af_vsock.c
> > @@ -107,6 +107,7 @@
> >  #include <linux/mutex.h>
> >  #include <linux/net.h>
> >  #include <linux/poll.h>
> > +#include <linux/random.h>
> >  #include <linux/skbuff.h>
> >  #include <linux/smp.h>
> >  #include <linux/socket.h>
> > @@ -504,9 +505,12 @@ static void vsock_pending_work(struct work_struct *work)
> >  static int __vsock_bind_stream(struct vsock_sock *vsk,
> >                              struct sockaddr_vm *addr)
> >  {
> > -     static u32 port = LAST_RESERVED_PORT + 1;
> > +     static u32 port = 0;
> >       struct sockaddr_vm new_addr;
> >
> > +     if (!port)
> > +             port = prandom_u32();
> > +

How about making this:
   port = LAST_RESERVED_PORT + 1 + prandom_u32_max(U32_MAX - LAST_RESERVED_PORT);
so the initial assignment is a valid port in the unreserved range. It will be corrected in the first iteration,
but this would make the intention clearer.

> >       vsock_addr_init(&new_addr, addr->svm_cid, addr->svm_port);
> >
> >       if (addr->svm_port == VMADDR_PORT_ANY) {
> > --
> > 2.20.0.rc2.403.gdbc3b29805-goog
>>

Thanks,
Jorgen