| 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
| ||
|
Message-ID: <aR0arw2F/DmbIrzY@devvm11784.nha0.facebook.com>
Date: Tue, 18 Nov 2025 17:17:35 -0800
From: Bobby Eshleman <bobbyeshleman@...il.com>
To: Stefano Garzarella <sgarzare@...hat.com>
Cc: "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Simon Horman <horms@...nel.org>,
Stefan Hajnoczi <stefanha@...hat.com>,
"Michael S. Tsirkin" <mst@...hat.com>,
Jason Wang <jasowang@...hat.com>,
Eugenio Pérez <eperezma@...hat.com>,
Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
"K. Y. Srinivasan" <kys@...rosoft.com>,
Haiyang Zhang <haiyangz@...rosoft.com>,
Wei Liu <wei.liu@...nel.org>, Dexuan Cui <decui@...rosoft.com>,
Bryan Tan <bryan-bt.tan@...adcom.com>,
Vishnu Dasa <vishnu.dasa@...adcom.com>,
Broadcom internal kernel review list <bcm-kernel-feedback-list@...adcom.com>,
Shuah Khan <shuah@...nel.org>, linux-kernel@...r.kernel.org,
virtualization@...ts.linux.dev, netdev@...r.kernel.org,
kvm@...r.kernel.org, linux-hyperv@...r.kernel.org,
linux-kselftest@...r.kernel.org, Sargun Dhillon <sargun@...gun.me>,
berrange@...hat.com, Bobby Eshleman <bobbyeshleman@...a.com>
Subject: Re: [PATCH net-next v10 03/11] vsock: reject bad
VSOCK_NET_MODE_LOCAL configuration for G2H
On Tue, Nov 18, 2025 at 07:10:28PM +0100, Stefano Garzarella wrote:
> On Mon, Nov 17, 2025 at 06:00:26PM -0800, Bobby Eshleman wrote:
> > From: Bobby Eshleman <bobbyeshleman@...a.com>
> >
> > diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c
> > index 2c937a2df83b..c8319cd1c232 100644
> > --- a/drivers/vhost/vsock.c
> > +++ b/drivers/vhost/vsock.c
> > @@ -64,6 +64,11 @@ static u32 vhost_transport_get_local_cid(void)
> > return VHOST_VSOCK_DEFAULT_HOST_CID;
> > }
> >
> > +static bool vhost_transport_supports_local_mode(void)
> > +{
> > + return true;
>
> Should we enable this later, when we really add support, or it doesn't
> affect anything if vhost-vsock is not really supporting it in this PR
> (thinking about bisection issues).
sgtm!
>
> > +}
> > +
> > /* Callers that dereference the return value must hold vhost_vsock_mutex or the
> > * RCU read lock.
> > */
> > @@ -412,6 +417,7 @@ static struct virtio_transport vhost_transport = {
> > .module = THIS_MODULE,
> >
> > .get_local_cid = vhost_transport_get_local_cid,
> > + .supports_local_mode = vhost_transport_supports_local_mode,
> >
> > .init = virtio_transport_do_socket_init,
> > .destruct = virtio_transport_destruct,
> > diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h
> > index 59d97a143204..824d89657d41 100644
> > --- a/include/net/af_vsock.h
> > +++ b/include/net/af_vsock.h
> > @@ -180,6 +180,11 @@ struct vsock_transport {
> > /* Addressing. */
> > u32 (*get_local_cid)(void);
> >
> > + /* Return true if this transport supports VSOCK_NET_MODE_LOCAL.
>
> nit: Here I would make it clearer that rather than supporting MODE_LOCAL,
> the transport is not compatible with it, etc.
> A summary of the excellent description we have in the commit.
>
sounds good!
> > + * Otherwise, return false.
> > + */
> > + bool (*supports_local_mode)(void);
> > +
> > /* Read a single skb */
> > int (*read_skb)(struct vsock_sock *, skb_read_actor_t);
> >
> > diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> > index 54373ae101c3..7a235bb94437 100644
> > --- a/net/vmw_vsock/af_vsock.c
> > +++ b/net/vmw_vsock/af_vsock.c
> > @@ -91,6 +91,12 @@
> > * and locked down by a namespace manager. The default is "global". The mode
> > * is set per-namespace.
> > *
> > + * Note: LOCAL mode is only supported when using namespace-aware transports
> > + * (vhost-vsock, loopback). If a guest-to-host transport (virtio-vsock,
> > + * hyperv-vsock, vmci-vsock) is loaded, attempts to set LOCAL mode will fail
> > + * with EOPNOTSUPP, as these transports do not support per-namespace
> > + * isolation.
>
> Okay, maybe this is fine, so if you don't need to resend, feel free to
> ignore the previous comment.
>
> > + *
> > * The modes affect the allocation and accessibility of CIDs as follows:
> > *
> > * - global - access and allocation are all system-wide
> > @@ -2765,17 +2771,30 @@ static int vsock_net_mode_string(const struct ctl_table *table, int write,
> > if (*lenp >= sizeof(data))
> > return -EINVAL;
> >
> > - if (!strncmp(data, VSOCK_NET_MODE_STR_GLOBAL, sizeof(data)))
> > + ret = 0;
>
> IIUC `ret` should already be 0 at this point, no?
>
> > + mutex_lock(&vsock_register_mutex);
>
> I honestly don't like to mix the parsing, with this new check, so what
> about leaving the parsing as before this patch (also without the mutex),
> then just (untested):
>
> mutex_lock(&vsock_register_mutex);
> if (mode == VSOCK_NET_MODE_LOCAL && transport_g2h &&
> transport_g2h->supports_local_mode &&
> !transport_g2h->supports_local_mode()) {
> ret = -EOPNOTSUPP;
> goto out;
> }
>
> if (!vsock_net_write_mode(net, mode)) {
> ret = -EPERM;
> }
> out:
> mutex_unlock(&vsock_register_mutex);
> return ret;
> }
Makes sense, I can move that around for next rev.
>
> > + if (!strncmp(data, VSOCK_NET_MODE_STR_GLOBAL, sizeof(data))) {
> > mode = VSOCK_NET_MODE_GLOBAL;
> > - else if (!strncmp(data, VSOCK_NET_MODE_STR_LOCAL, sizeof(data)))
> > + } else if (!strncmp(data, VSOCK_NET_MODE_STR_LOCAL, sizeof(data))) {
> > + if (transport_g2h && transport_g2h->supports_local_mode &&
> > + !transport_g2h->supports_local_mode()) {
> > + ret = -EOPNOTSUPP;
> > + goto out;
> > + }
> > mode = VSOCK_NET_MODE_LOCAL;
> > - else
> > - return -EINVAL;
> > + } else {
> > + ret = -EINVAL;
> > + goto out;
> > + }
> >
> > - if (!vsock_net_write_mode(net, mode))
> > - return -EPERM;
> > + if (!vsock_net_write_mode(net, mode)) {
> > + ret = -EPERM;
> > + goto out;
> > + }
> >
> > - return 0;
> > +out:
> > + mutex_unlock(&vsock_register_mutex);
> > + return ret;
> > }
> >
> > static struct ctl_table vsock_table[] = {
> > @@ -2916,6 +2935,7 @@ int vsock_core_register(const struct vsock_transport *t, int features)
> > {
> > const struct vsock_transport *t_h2g, *t_g2h, *t_dgram, *t_local;
> > int err = mutex_lock_interruptible(&vsock_register_mutex);
> > + struct net *net;
> >
> > if (err)
> > return err;
> > @@ -2938,6 +2958,22 @@ int vsock_core_register(const struct vsock_transport *t, int features)
> > err = -EBUSY;
> > goto err_busy;
> > }
> > +
> > + /* G2H sockets break in LOCAL mode namespaces because G2H
> > + * transports don't support them yet. Block registering new G2H
> > + * transports if we already have local mode namespaces on the
> > + * system.
> > + */
> > + rcu_read_lock();
> > + for_each_net_rcu(net) {
> > + if (vsock_net_mode(net) == VSOCK_NET_MODE_LOCAL) {
> > + rcu_read_unlock();
> > + err = -EOPNOTSUPP;
> > + goto err_busy;
> > + }
> > + }
> > + rcu_read_unlock();
> > +
> > t_g2h = t;
> > }
> >
> > diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
> > index 432fcbbd14d4..279f04fcd81a 100644
> > --- a/net/vmw_vsock/hyperv_transport.c
> > +++ b/net/vmw_vsock/hyperv_transport.c
> > @@ -833,10 +833,16 @@ int hvs_notify_set_rcvlowat(struct vsock_sock *vsk, int val)
> > return -EOPNOTSUPP;
> > }
> >
> > +static bool hvs_supports_local_mode(void)
> > +{
> > + return false;
> > +}
> > +
> > static struct vsock_transport hvs_transport = {
> > .module = THIS_MODULE,
> >
> > .get_local_cid = hvs_get_local_cid,
> > + .supports_local_mode = hvs_supports_local_mode,
> >
> > .init = hvs_sock_init,
> > .destruct = hvs_destruct,
> > diff --git a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
> > index 5d379ccf3770..e585cb66c6f5 100644
> > --- a/net/vmw_vsock/virtio_transport.c
> > +++ b/net/vmw_vsock/virtio_transport.c
> > @@ -94,6 +94,18 @@ static u32 virtio_transport_get_local_cid(void)
> > return ret;
> > }
> >
> > +static bool virtio_transport_supports_local_mode(void)
> > +{
> > + struct virtio_vsock *vsock;
> > +
> > + rcu_read_lock();
> > + vsock = rcu_dereference(the_virtio_vsock);
> > + rcu_read_unlock();
> > +
> > + /* Local mode is supported only when no G2H device is present. */
> > + return vsock ? false : true;
> > +}
> > +
> > /* Caller need to hold vsock->tx_lock on vq */
> > static int virtio_transport_send_skb(struct sk_buff *skb, struct virtqueue *vq,
> > struct virtio_vsock *vsock, gfp_t gfp)
> > @@ -544,6 +556,7 @@ static struct virtio_transport virtio_transport = {
> > .module = THIS_MODULE,
> >
> > .get_local_cid = virtio_transport_get_local_cid,
> > + .supports_local_mode = virtio_transport_supports_local_mode,
> >
> > .init = virtio_transport_do_socket_init,
> > .destruct = virtio_transport_destruct,
> > diff --git a/net/vmw_vsock/vmci_transport.c b/net/vmw_vsock/vmci_transport.c
> > index 7eccd6708d66..da7c52ad7b2a 100644
> > --- a/net/vmw_vsock/vmci_transport.c
> > +++ b/net/vmw_vsock/vmci_transport.c
> > @@ -2033,6 +2033,12 @@ static u32 vmci_transport_get_local_cid(void)
> > return vmci_get_context_id();
> > }
> >
> > +static bool vmci_transport_supports_local_mode(void)
> > +{
> > + /* Local mode is supported only when no device is present. */
> > + return vmci_transport_get_local_cid() == VMCI_INVALID_ID;
>
> IIRC vmci can be registered both as H2G and G2H, so should we filter out
> the H2G case?
In fact, I'm realizing now that this should probably just be:
static bool vmci_transport_supports_local_mode(void)
{
return false;
}
... because even for H2G there is no mechanism for attaching a namespace
to a VM (unlike w/ vhost_vsock device open).
Does that seem right?
Best,
Bobby
Powered by blists - more mailing lists