Re: [PATCH virt] virt: fix uninit-value in vhost_vsock_dev_open
> But now that it's explained, the bugfix as proposed is incomplete: > userspace can set features twice and the second time will leak > old VIRTIO_VSOCK_F_SEQPACKET bit value. > > And I am pretty sure the Fixes tag is wrong. > > So I wrote this, but I actually don't have a set for > seqpacket to test this. Arseny could you help test maybe? > Thanks! Hi! Sorry for late reply! Just run vsock test suite with this patch - seems everything is ok! > > > commit bcc17a060d93b198d8a17a9b87b593f41337ee28 > Author: Michael S. Tsirkin > Date: Mon Apr 22 10:03:13 2024 -0400 > > vhost/vsock: always initialize seqpacket_allow > > There are two issues around seqpacket_allow: > 1. seqpacket_allow is not initialized when socket is > created. Thus if features are never set, it will be > read uninitialized. > 2. if VIRTIO_VSOCK_F_SEQPACKET is set and then cleared, > then seqpacket_allow will not be cleared appropriately > (existing apps I know about don't usually do this but > it's legal and there's no way to be sure no one relies > on this). > > To fix: > - initialize seqpacket_allow after allocation > - set it unconditionally in set_features > > Reported-by: syzbot+6c21aeb59d0e82eb2...@syzkaller.appspotmail.com > Reported-by: Jeongjun Park > Fixes: ced7b713711f ("vhost/vsock: support SEQPACKET for transport"). > Cc: Arseny Krasnov > Cc: David S. Miller > Cc: Stefan Hajnoczi > Signed-off-by: Michael S. Tsirkin Acked-by: Arseniy Krasnov > > diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c > index ec20ecff85c7..bf664ec9341b 100644 > --- a/drivers/vhost/vsock.c > +++ b/drivers/vhost/vsock.c > @@ -667,6 +667,7 @@ static int vhost_vsock_dev_open(struct inode *inode, > struct file *file) > } > > vsock->guest_cid = 0; /* no CID assigned yet */ > + vsock->seqpacket_allow = false; > > atomic_set(>queued_replies, 0); > > @@ -810,8 +811,7 @@ static int vhost_vsock_set_features(struct vhost_vsock > *vsock, u64 features) > goto err; > } > > - if (features & (1ULL << VIRTIO_VSOCK_F_SEQPACKET)) > - vsock->seqpacket_allow = true; > + vsock->seqpacket_allow = features & (1ULL << VIRTIO_VSOCK_F_SEQPACKET); > > for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) { > vq = >vqs[i];
Re: [PATCH virt] virt: fix uninit-value in vhost_vsock_dev_open
On Mon, Apr 22, 2024 at 09:00:31AM -0400, Stefan Hajnoczi wrote: > On Sun, Apr 21, 2024 at 12:06:06PM +0900, Jeongjun Park wrote: > > static bool vhost_transport_seqpacket_allow(u32 remote_cid) > > { > > > > vsock = vhost_vsock_get(remote_cid); > > > > if (vsock) > > seqpacket_allow = vsock->seqpacket_allow; > > > > } > > > > I think this is due to reading a previously created uninitialized > > vsock->seqpacket_allow inside vhost_transport_seqpacket_allow(), > > which is executed by the function pointer present in the if statement. > > CCing Arseny, author of commit ced7b713711f ("vhost/vsock: support > SEQPACKET for transport"). > > Looks like a genuine bug in the commit. vhost_vsock_set_features() sets > seqpacket_allow to true when the feature is negotiated. The assumption > is that the field defaults to false. > > The rest of the vhost_vsock.ko code is written to initialize the > vhost_vsock fields, so you could argue seqpacket_allow should just be > explicitly initialized to false. > > However, eliminating this class of errors by zeroing seems reasonable in > this code path. vhost_vsock_dev_open() is not performance-critical. > > Acked-by: Stefan Hajnoczi But now that it's explained, the bugfix as proposed is incomplete: userspace can set features twice and the second time will leak old VIRTIO_VSOCK_F_SEQPACKET bit value. And I am pretty sure the Fixes tag is wrong. So I wrote this, but I actually don't have a set for seqpacket to test this. Arseny could you help test maybe? Thanks! commit bcc17a060d93b198d8a17a9b87b593f41337ee28 Author: Michael S. Tsirkin Date: Mon Apr 22 10:03:13 2024 -0400 vhost/vsock: always initialize seqpacket_allow There are two issues around seqpacket_allow: 1. seqpacket_allow is not initialized when socket is created. Thus if features are never set, it will be read uninitialized. 2. if VIRTIO_VSOCK_F_SEQPACKET is set and then cleared, then seqpacket_allow will not be cleared appropriately (existing apps I know about don't usually do this but it's legal and there's no way to be sure no one relies on this). To fix: - initialize seqpacket_allow after allocation - set it unconditionally in set_features Reported-by: syzbot+6c21aeb59d0e82eb2...@syzkaller.appspotmail.com Reported-by: Jeongjun Park Fixes: ced7b713711f ("vhost/vsock: support SEQPACKET for transport"). Cc: Arseny Krasnov Cc: David S. Miller Cc: Stefan Hajnoczi Signed-off-by: Michael S. Tsirkin diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c index ec20ecff85c7..bf664ec9341b 100644 --- a/drivers/vhost/vsock.c +++ b/drivers/vhost/vsock.c @@ -667,6 +667,7 @@ static int vhost_vsock_dev_open(struct inode *inode, struct file *file) } vsock->guest_cid = 0; /* no CID assigned yet */ + vsock->seqpacket_allow = false; atomic_set(>queued_replies, 0); @@ -810,8 +811,7 @@ static int vhost_vsock_set_features(struct vhost_vsock *vsock, u64 features) goto err; } - if (features & (1ULL << VIRTIO_VSOCK_F_SEQPACKET)) - vsock->seqpacket_allow = true; + vsock->seqpacket_allow = features & (1ULL << VIRTIO_VSOCK_F_SEQPACKET); for (i = 0; i < ARRAY_SIZE(vsock->vqs); i++) { vq = >vqs[i];
Re: [PATCH virt] virt: fix uninit-value in vhost_vsock_dev_open
On Sun, Apr 21, 2024 at 12:06:06PM +0900, Jeongjun Park wrote: > static bool vhost_transport_seqpacket_allow(u32 remote_cid) > { > > vsock = vhost_vsock_get(remote_cid); > > if (vsock) > seqpacket_allow = vsock->seqpacket_allow; > > } > > I think this is due to reading a previously created uninitialized > vsock->seqpacket_allow inside vhost_transport_seqpacket_allow(), > which is executed by the function pointer present in the if statement. CCing Arseny, author of commit ced7b713711f ("vhost/vsock: support SEQPACKET for transport"). Looks like a genuine bug in the commit. vhost_vsock_set_features() sets seqpacket_allow to true when the feature is negotiated. The assumption is that the field defaults to false. The rest of the vhost_vsock.ko code is written to initialize the vhost_vsock fields, so you could argue seqpacket_allow should just be explicitly initialized to false. However, eliminating this class of errors by zeroing seems reasonable in this code path. vhost_vsock_dev_open() is not performance-critical. Acked-by: Stefan Hajnoczi signature.asc Description: PGP signature
Re: [PATCH virt] virt: fix uninit-value in vhost_vsock_dev_open
static bool vhost_transport_seqpacket_allow(u32 remote_cid) { vsock = vhost_vsock_get(remote_cid); if (vsock) seqpacket_allow = vsock->seqpacket_allow; } I think this is due to reading a previously created uninitialized vsock->seqpacket_allow inside vhost_transport_seqpacket_allow(), which is executed by the function pointer present in the if statement. Thanks
Re: [PATCH virt] virt: fix uninit-value in vhost_vsock_dev_open
On Sat, Apr 20, 2024 at 05:57:50PM +0900, Jeongjun Park wrote: > Change vhost_vsock_dev_open() to use kvzalloc() instead of kvmalloc() > to avoid uninit state. > > Reported-by: syzbot+6c21aeb59d0e82eb2...@syzkaller.appspotmail.com > Fixes: dcda9b04713c ("mm, tree wide: replace __GFP_REPEAT by > __GFP_RETRY_MAYFAIL with more useful semantic") > Signed-off-by: Jeongjun Park What value exactly is used uninitialized? > --- > drivers/vhost/vsock.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c > index ec20ecff85c7..652ef97a444b 100644 > --- a/drivers/vhost/vsock.c > +++ b/drivers/vhost/vsock.c > @@ -656,7 +656,7 @@ static int vhost_vsock_dev_open(struct inode *inode, > struct file *file) > /* This struct is large and allocation could fail, fall back to vmalloc >* if there is no other way. >*/ > - vsock = kvmalloc(sizeof(*vsock), GFP_KERNEL | __GFP_RETRY_MAYFAIL); > + vsock = kvzalloc(sizeof(*vsock), GFP_KERNEL | __GFP_RETRY_MAYFAIL); > if (!vsock) > return -ENOMEM; > > -- > 2.34.1