1. Patchew
  2. linux
  3. [PATCH RFC net v2 0/3] vsock: Fix transport_{h2g,g2h,dgram,local} TOCTOU issues
  4. View patch

[PATCH RFC net v2 3/3] vsock: Fix IOCTL_VM_SOCKETS_GET_LOCAL_CID to check also `transport_local`

Michal Luczaj posted 3 patches 3 months, 2 weeks ago
There is a newer version of this series
[PATCH RFC net v2 3/3] vsock: Fix IOCTL_VM_SOCKETS_GET_LOCAL_CID to check also `transport_local`
Posted by Michal Luczaj 3 months, 2 weeks ago 
Support returning VMADDR_CID_LOCAL in case no other vsock transport is
available.

Fixes: 0e12190578d0 ("vsock: add local transport support in the vsock core")
Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
man vsock(7) mentions IOCTL_VM_SOCKETS_GET_LOCAL_CID vs. VMADDR_CID_LOCAL:

   Ioctls
       ...
       IOCTL_VM_SOCKETS_GET_LOCAL_CID
              ...
              Consider using VMADDR_CID_ANY when binding instead of
              getting the local CID with IOCTL_VM_SOCKETS_GET_LOCAL_CID.

   Local communication
       ....
       The local CID obtained with IOCTL_VM_SOCKETS_GET_LOCAL_CID can be
       used for the same purpose, but it is preferable to use
       VMADDR_CID_LOCAL.

I was wondering it that would need some rewriting, since we're adding
VMADDR_CID_LOCAL as a possible ioctl's return value.
---
 net/vmw_vsock/af_vsock.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
index a1b1073a2c89f865fcdb58b38d8e7feffcf1544f..4bdb4016bd14d790f3d217d5063be64a1553b194 100644
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -2577,6 +2577,8 @@ static long vsock_dev_do_ioctl(struct file *filp,
 		cid = vsock_transport_local_cid(&transport_g2h);
 		if (cid == VMADDR_CID_ANY)
 			cid = vsock_transport_local_cid(&transport_h2g);
+		if (cid == VMADDR_CID_ANY && transport_local)
+			cid = VMADDR_CID_LOCAL;
 
 		if (put_user(cid, p) != 0)
 			retval = -EFAULT;

-- 
2.49.0
Re: [PATCH RFC net v2 3/3] vsock: Fix IOCTL_VM_SOCKETS_GET_LOCAL_CID to check also `transport_local`
Posted by Stefano Garzarella 3 months, 2 weeks ago 
On Fri, Jun 20, 2025 at 09:52:45PM +0200, Michal Luczaj wrote:
>Support returning VMADDR_CID_LOCAL in case no other vsock transport is
>available.
>
>Fixes: 0e12190578d0 ("vsock: add local transport support in the vsock core")
>Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>Signed-off-by: Michal Luczaj <mhal@rbox.co>
>---
>man vsock(7) mentions IOCTL_VM_SOCKETS_GET_LOCAL_CID vs. VMADDR_CID_LOCAL:
>
>   Ioctls
>       ...
>       IOCTL_VM_SOCKETS_GET_LOCAL_CID
>              ...
>              Consider using VMADDR_CID_ANY when binding instead of
>              getting the local CID with IOCTL_VM_SOCKETS_GET_LOCAL_CID.
>
>   Local communication
>       ....
>       The local CID obtained with IOCTL_VM_SOCKETS_GET_LOCAL_CID can be
>       used for the same purpose, but it is preferable to use
>       VMADDR_CID_LOCAL.
>
>I was wondering it that would need some rewriting, since we're adding
>VMADDR_CID_LOCAL as a possible ioctl's return value.

IIRC the reason was, that if we have for example a G2H module loaded, 
the ioctl returns the CID of that module (e.g. 42). So, we can use both 
42 and VMADDR_CID_LOCAL to do the loopback communication, but we 
encourage to always use VMADDR_CID_LOCAL.  With this change we basically 
don't change that, but we change the fact that if there is only the 
loopback module loaded, before the ioctl returned VMADDR_CID_ANY, while 
now it returns LOCAL rightly.

So, IMO we are fine.

>---
> net/vmw_vsock/af_vsock.c | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>index a1b1073a2c89f865fcdb58b38d8e7feffcf1544f..4bdb4016bd14d790f3d217d5063be64a1553b194 100644
>--- a/net/vmw_vsock/af_vsock.c
>+++ b/net/vmw_vsock/af_vsock.c
>@@ -2577,6 +2577,8 @@ static long vsock_dev_do_ioctl(struct file *filp,
> 		cid = vsock_transport_local_cid(&transport_g2h);
> 		if (cid == VMADDR_CID_ANY)
> 			cid = vsock_transport_local_cid(&transport_h2g);
>+		if (cid == VMADDR_CID_ANY && transport_local)
>+			cid = VMADDR_CID_LOCAL;

why not `cid = vsock_transport_local_cid(&transport_local)` like for 
H2G?

Thanks,
Stefano

>
> 		if (put_user(cid, p) != 0)
> 			retval = -EFAULT;
>
>-- 
>2.49.0
>
Re: [PATCH RFC net v2 3/3] vsock: Fix IOCTL_VM_SOCKETS_GET_LOCAL_CID to check also `transport_local`
Posted by Michal Luczaj 3 months, 2 weeks ago 
On 6/25/25 10:54, Stefano Garzarella wrote:
> On Fri, Jun 20, 2025 at 09:52:45PM +0200, Michal Luczaj wrote:
>> Support returning VMADDR_CID_LOCAL in case no other vsock transport is
>> available.
>>
>> Fixes: 0e12190578d0 ("vsock: add local transport support in the vsock core")
>> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>> ---
>> man vsock(7) mentions IOCTL_VM_SOCKETS_GET_LOCAL_CID vs. VMADDR_CID_LOCAL:
>>
>>   Ioctls
>>       ...
>>       IOCTL_VM_SOCKETS_GET_LOCAL_CID
>>              ...
>>              Consider using VMADDR_CID_ANY when binding instead of
>>              getting the local CID with IOCTL_VM_SOCKETS_GET_LOCAL_CID.
>>
>>   Local communication
>>       ....
>>       The local CID obtained with IOCTL_VM_SOCKETS_GET_LOCAL_CID can be
>>       used for the same purpose, but it is preferable to use
>>       VMADDR_CID_LOCAL.
>>
>> I was wondering it that would need some rewriting, since we're adding
>> VMADDR_CID_LOCAL as a possible ioctl's return value.
> 
> IIRC the reason was, that if we have for example a G2H module loaded, 
> the ioctl returns the CID of that module (e.g. 42). So, we can use both 
> 42 and VMADDR_CID_LOCAL to do the loopback communication, but we 
> encourage to always use VMADDR_CID_LOCAL.  With this change we basically 
> don't change that, but we change the fact that if there is only the 
> loopback module loaded, before the ioctl returned VMADDR_CID_ANY, while 
> now it returns LOCAL rightly.
> 
> So, IMO we are fine.

All right, got it.

>> ---
>> net/vmw_vsock/af_vsock.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>> index a1b1073a2c89f865fcdb58b38d8e7feffcf1544f..4bdb4016bd14d790f3d217d5063be64a1553b194 100644
>> --- a/net/vmw_vsock/af_vsock.c
>> +++ b/net/vmw_vsock/af_vsock.c
>> @@ -2577,6 +2577,8 @@ static long vsock_dev_do_ioctl(struct file *filp,
>> 		cid = vsock_transport_local_cid(&transport_g2h);
>> 		if (cid == VMADDR_CID_ANY)
>> 			cid = vsock_transport_local_cid(&transport_h2g);
>> +		if (cid == VMADDR_CID_ANY && transport_local)
>> +			cid = VMADDR_CID_LOCAL;
> 
> why not `cid = vsock_transport_local_cid(&transport_local)` like for 
> H2G?

Sure, can do. I've assumed transport_local would always have a local CID of
VMADDR_CID_LOCAL. So taking mutex and going through a callback function to
get VMADDR_CID_LOCAL seemed superfluous. But I get it, if you want to have
it symmetrical with the other vsock_transport_local_cid()s.

Thanks,
Michal
Re: [PATCH RFC net v2 3/3] vsock: Fix IOCTL_VM_SOCKETS_GET_LOCAL_CID to check also `transport_local`
Posted by Stefano Garzarella 3 months, 1 week ago 
On Wed, Jun 25, 2025 at 11:23:54PM +0200, Michal Luczaj wrote:
>On 6/25/25 10:54, Stefano Garzarella wrote:
>> On Fri, Jun 20, 2025 at 09:52:45PM +0200, Michal Luczaj wrote:
>>> Support returning VMADDR_CID_LOCAL in case no other vsock transport is
>>> available.
>>>
>>> Fixes: 0e12190578d0 ("vsock: add local transport support in the vsock core")
>>> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>> ---
>>> man vsock(7) mentions IOCTL_VM_SOCKETS_GET_LOCAL_CID vs. VMADDR_CID_LOCAL:
>>>
>>>   Ioctls
>>>       ...
>>>       IOCTL_VM_SOCKETS_GET_LOCAL_CID
>>>              ...
>>>              Consider using VMADDR_CID_ANY when binding instead of
>>>              getting the local CID with IOCTL_VM_SOCKETS_GET_LOCAL_CID.
>>>
>>>   Local communication
>>>       ....
>>>       The local CID obtained with IOCTL_VM_SOCKETS_GET_LOCAL_CID can be
>>>       used for the same purpose, but it is preferable to use
>>>       VMADDR_CID_LOCAL.
>>>
>>> I was wondering it that would need some rewriting, since we're adding
>>> VMADDR_CID_LOCAL as a possible ioctl's return value.
>>
>> IIRC the reason was, that if we have for example a G2H module loaded,
>> the ioctl returns the CID of that module (e.g. 42). So, we can use both
>> 42 and VMADDR_CID_LOCAL to do the loopback communication, but we
>> encourage to always use VMADDR_CID_LOCAL.  With this change we basically
>> don't change that, but we change the fact that if there is only the
>> loopback module loaded, before the ioctl returned VMADDR_CID_ANY, while
>> now it returns LOCAL rightly.
>>
>> So, IMO we are fine.
>
>All right, got it.
>
>>> ---
>>> net/vmw_vsock/af_vsock.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>>
>>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>>> index a1b1073a2c89f865fcdb58b38d8e7feffcf1544f..4bdb4016bd14d790f3d217d5063be64a1553b194 100644
>>> --- a/net/vmw_vsock/af_vsock.c
>>> +++ b/net/vmw_vsock/af_vsock.c
>>> @@ -2577,6 +2577,8 @@ static long vsock_dev_do_ioctl(struct file *filp,
>>> 		cid = vsock_transport_local_cid(&transport_g2h);
>>> 		if (cid == VMADDR_CID_ANY)
>>> 			cid = vsock_transport_local_cid(&transport_h2g);
>>> +		if (cid == VMADDR_CID_ANY && transport_local)
>>> +			cid = VMADDR_CID_LOCAL;
>>
>> why not `cid = vsock_transport_local_cid(&transport_local)` like for
>> H2G?
>
>Sure, can do. I've assumed transport_local would always have a local CID of
>VMADDR_CID_LOCAL. So taking mutex and going through a callback function to
>get VMADDR_CID_LOCAL seemed superfluous. But I get it, if you want to have
>it symmetrical with the other vsock_transport_local_cid()s.

Yeah, BTW for transport_h2g is the same, they always should return 
VMADDR_CID_HOST, so I think we should be symmetrical.

Thanks,
Stefano
Re: [PATCH RFC net v2 3/3] vsock: Fix IOCTL_VM_SOCKETS_GET_LOCAL_CID to check also `transport_local`
Posted by Michal Luczaj 3 months, 1 week ago 
On 6/27/25 10:10, Stefano Garzarella wrote:
> On Wed, Jun 25, 2025 at 11:23:54PM +0200, Michal Luczaj wrote:
>> On 6/25/25 10:54, Stefano Garzarella wrote:
>>> On Fri, Jun 20, 2025 at 09:52:45PM +0200, Michal Luczaj wrote:
>>>> Support returning VMADDR_CID_LOCAL in case no other vsock transport is
>>>> available.
>>>>
>>>> Fixes: 0e12190578d0 ("vsock: add local transport support in the vsock core")
>>>> Suggested-by: Stefano Garzarella <sgarzare@redhat.com>
>>>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>>>> ---
>>>> man vsock(7) mentions IOCTL_VM_SOCKETS_GET_LOCAL_CID vs. VMADDR_CID_LOCAL:
>>>>
>>>>   Ioctls
>>>>       ...
>>>>       IOCTL_VM_SOCKETS_GET_LOCAL_CID
>>>>              ...
>>>>              Consider using VMADDR_CID_ANY when binding instead of
>>>>              getting the local CID with IOCTL_VM_SOCKETS_GET_LOCAL_CID.
>>>>
>>>>   Local communication
>>>>       ....
>>>>       The local CID obtained with IOCTL_VM_SOCKETS_GET_LOCAL_CID can be
>>>>       used for the same purpose, but it is preferable to use
>>>>       VMADDR_CID_LOCAL.
>>>>
>>>> I was wondering it that would need some rewriting, since we're adding
>>>> VMADDR_CID_LOCAL as a possible ioctl's return value.
>>>
>>> IIRC the reason was, that if we have for example a G2H module loaded,
>>> the ioctl returns the CID of that module (e.g. 42). So, we can use both
>>> 42 and VMADDR_CID_LOCAL to do the loopback communication, but we
>>> encourage to always use VMADDR_CID_LOCAL.  With this change we basically
>>> don't change that, but we change the fact that if there is only the
>>> loopback module loaded, before the ioctl returned VMADDR_CID_ANY, while
>>> now it returns LOCAL rightly.
>>>
>>> So, IMO we are fine.
>>
>> All right, got it.
>>
>>>> ---
>>>> net/vmw_vsock/af_vsock.c | 2 ++
>>>> 1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
>>>> index a1b1073a2c89f865fcdb58b38d8e7feffcf1544f..4bdb4016bd14d790f3d217d5063be64a1553b194 100644
>>>> --- a/net/vmw_vsock/af_vsock.c
>>>> +++ b/net/vmw_vsock/af_vsock.c
>>>> @@ -2577,6 +2577,8 @@ static long vsock_dev_do_ioctl(struct file *filp,
>>>> 		cid = vsock_transport_local_cid(&transport_g2h);
>>>> 		if (cid == VMADDR_CID_ANY)
>>>> 			cid = vsock_transport_local_cid(&transport_h2g);
>>>> +		if (cid == VMADDR_CID_ANY && transport_local)
>>>> +			cid = VMADDR_CID_LOCAL;
>>>
>>> why not `cid = vsock_transport_local_cid(&transport_local)` like for
>>> H2G?
>>
>> Sure, can do. I've assumed transport_local would always have a local CID of
>> VMADDR_CID_LOCAL. So taking mutex and going through a callback function to
>> get VMADDR_CID_LOCAL seemed superfluous. But I get it, if you want to have
>> it symmetrical with the other vsock_transport_local_cid()s.
> 
> Yeah, BTW for transport_h2g is the same, they always should return 
> VMADDR_CID_HOST, so I think we should be symmetrical.

Heh, I've missed that VMADDR_CID_HOST completely :)

Thanks,
Michal

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.