[REGRESSION] virtio: reject shm region if length is zero

[REGRESSION] virtio: reject shm region if length is zero

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 2148 bytes --]

On Mon, May 12, 2025 at 07:51:53AM +0930, SamiUddinsami.md.ko@gmail.com wrote:
> From: Sami Uddin <sami.md.ko@gmail.com>
>
> Prevent usage of shared memory regions where the length is zero,
> as such configurations are not valid and may lead to unexpected behavior.
>
> Signed-off-by: Sami Uddin <sami.md.ko@gmail.com>
> ---
> v3:
> - Use idiomatic 'if (!region->len)' as suggested by reviewer
> v2:
> - Fixed coding style issue: added space after 'if' statement
>
>  include/linux/virtio_config.h | 2 ++
>  1 file changed, 2 insertions(+)

Hi, I'm sorry to be the bearer of bad news, but since this patch my VM
no longer works.  The system is running wayland-proxy-virtwl[1] inside
a crosvm[2] VM, using crosvm's virtio-gpu device to do cross-domain
Wayland forwarding.

Since this change, wayland-proxy-virtwl crashes with the following log
message:

	wl-proxy [WARNING]: Error handling client: Unix.Unix_error(Unix.EINVAL, "DRM_IOCTL_VIRTGPU_RESOURCE_CREATE_BLOB", "")

I'm pretty confused by what this change was supposed to do in the first
place…  Looking at how virtio_get_shm_region() is used in
virtio_gpu_init(), it's called with a pointer to zeroed memory, and then
the get_shm_region() implementation is supposed to write to the region,
without ever reading from it as far as I can tell.  Why is the initial
value of an out parameter being checked at all?  How does this prevent
using zero-length shared memory regions?

[1]: https://crosvm.dev/
[2]: https://github.com/talex5/wayland-proxy-virtwl

#regzbot introduced: 206cc44588f72b49ad4d7e21a7472ab2a72a83df

> diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
> index 169c7d367fac..b3e1d30c765b 100644
> --- a/include/linux/virtio_config.h
> +++ b/include/linux/virtio_config.h
> @@ -329,6 +329,8 @@ static inline
>  bool virtio_get_shm_region(struct virtio_device *vdev,
>  			   struct virtio_shm_region *region, u8 id)
>  {
> +	if (!region->len)
> +		return false;
>  	if (!vdev->config->get_shm_region)
>  		return false;
>  	return vdev->config->get_shm_region(vdev, region, id);
> --
> 2.34.1
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [REGRESSION] virtio: reject shm region if length is zero

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 1969 bytes --]

Alyssa Ross <hi@alyssa.is> writes:

> On Mon, May 12, 2025 at 07:51:53AM +0930, SamiUddinsami.md.ko@gmail.com wrote:
>> From: Sami Uddin <sami.md.ko@gmail.com>
>>
>> Prevent usage of shared memory regions where the length is zero,
>> as such configurations are not valid and may lead to unexpected behavior.
>>
>> Signed-off-by: Sami Uddin <sami.md.ko@gmail.com>
>> ---
>> v3:
>> - Use idiomatic 'if (!region->len)' as suggested by reviewer
>> v2:
>> - Fixed coding style issue: added space after 'if' statement
>>
>>  include/linux/virtio_config.h | 2 ++
>>  1 file changed, 2 insertions(+)
>
> Hi, I'm sorry to be the bearer of bad news, but since this patch my VM
> no longer works.  The system is running wayland-proxy-virtwl[1] inside
> a crosvm[2] VM, using crosvm's virtio-gpu device to do cross-domain
> Wayland forwarding.
>
> Since this change, wayland-proxy-virtwl crashes with the following log
> message:
>
> 	wl-proxy [WARNING]: Error handling client: Unix.Unix_error(Unix.EINVAL, "DRM_IOCTL_VIRTGPU_RESOURCE_CREATE_BLOB", "")
>
> I'm pretty confused by what this change was supposed to do in the first
> place…  Looking at how virtio_get_shm_region() is used in
> virtio_gpu_init(), it's called with a pointer to zeroed memory, and then
> the get_shm_region() implementation is supposed to write to the region,
> without ever reading from it as far as I can tell.  Why is the initial
> value of an out parameter being checked at all?  How does this prevent
> using zero-length shared memory regions?
>
> [1]: https://crosvm.dev/
> [2]: https://github.com/talex5/wayland-proxy-virtwl
>
> #regzbot introduced: 206cc44588f72b49ad4d7e21a7472ab2a72a83df

Okay, just found that it's already been reverted:
https://lore.kernel.org/all/20250808072533-mutt-send-email-mst@kernel.org/

Still, I'm confused how this was supposed to fix anything…

#regzbot fix: Revert "virtio: reject shm region if length is zero"

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: [REGRESSION] virtio: reject shm region if length is zero

[Michael S. Tsirkin]

On Fri, Aug 15, 2025 at 09:19:34PM +0200, Alyssa Ross wrote:
> Alyssa Ross <hi@alyssa.is> writes:
> 
> > On Mon, May 12, 2025 at 07:51:53AM +0930, SamiUddinsami.md.ko@gmail.com wrote:
> >> From: Sami Uddin <sami.md.ko@gmail.com>
> >>
> >> Prevent usage of shared memory regions where the length is zero,
> >> as such configurations are not valid and may lead to unexpected behavior.
> >>
> >> Signed-off-by: Sami Uddin <sami.md.ko@gmail.com>
> >> ---
> >> v3:
> >> - Use idiomatic 'if (!region->len)' as suggested by reviewer
> >> v2:
> >> - Fixed coding style issue: added space after 'if' statement
> >>
> >>  include/linux/virtio_config.h | 2 ++
> >>  1 file changed, 2 insertions(+)
> >
> > Hi, I'm sorry to be the bearer of bad news, but since this patch my VM
> > no longer works.  The system is running wayland-proxy-virtwl[1] inside
> > a crosvm[2] VM, using crosvm's virtio-gpu device to do cross-domain
> > Wayland forwarding.
> >
> > Since this change, wayland-proxy-virtwl crashes with the following log
> > message:
> >
> > 	wl-proxy [WARNING]: Error handling client: Unix.Unix_error(Unix.EINVAL, "DRM_IOCTL_VIRTGPU_RESOURCE_CREATE_BLOB", "")
> >
> > I'm pretty confused by what this change was supposed to do in the first
> > place…  Looking at how virtio_get_shm_region() is used in
> > virtio_gpu_init(), it's called with a pointer to zeroed memory, and then
> > the get_shm_region() implementation is supposed to write to the region,
> > without ever reading from it as far as I can tell.  Why is the initial
> > value of an out parameter being checked at all?  How does this prevent
> > using zero-length shared memory regions?
> >
> > [1]: https://crosvm.dev/
> > [2]: https://github.com/talex5/wayland-proxy-virtwl
> >
> > #regzbot introduced: 206cc44588f72b49ad4d7e21a7472ab2a72a83df
> 
> Okay, just found that it's already been reverted:
> https://lore.kernel.org/all/20250808072533-mutt-send-email-mst@kernel.org/
> 
> Still, I'm confused how this was supposed to fix anything…
> 
> #regzbot fix: Revert "virtio: reject shm region if length is zero"



Are you asking why was the patch applied in the 1st place?
It seemed like an invalid behaviour to me, and I thought it's
not too late to block it so we don't need to support it
down the road.

-- 
MST

Re: [REGRESSION] virtio: reject shm region if length is zero

[Alyssa Ross]

[-- Attachment #1: Type: text/plain, Size: 2616 bytes --]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Fri, Aug 15, 2025 at 09:19:34PM +0200, Alyssa Ross wrote:
>> Alyssa Ross <hi@alyssa.is> writes:
>> 
>> > On Mon, May 12, 2025 at 07:51:53AM +0930, SamiUddinsami.md.ko@gmail.com wrote:
>> >> From: Sami Uddin <sami.md.ko@gmail.com>
>> >>
>> >> Prevent usage of shared memory regions where the length is zero,
>> >> as such configurations are not valid and may lead to unexpected behavior.
>> >>
>> >> Signed-off-by: Sami Uddin <sami.md.ko@gmail.com>
>> >> ---
>> >> v3:
>> >> - Use idiomatic 'if (!region->len)' as suggested by reviewer
>> >> v2:
>> >> - Fixed coding style issue: added space after 'if' statement
>> >>
>> >>  include/linux/virtio_config.h | 2 ++
>> >>  1 file changed, 2 insertions(+)
>> >
>> > Hi, I'm sorry to be the bearer of bad news, but since this patch my VM
>> > no longer works.  The system is running wayland-proxy-virtwl[1] inside
>> > a crosvm[2] VM, using crosvm's virtio-gpu device to do cross-domain
>> > Wayland forwarding.
>> >
>> > Since this change, wayland-proxy-virtwl crashes with the following log
>> > message:
>> >
>> > 	wl-proxy [WARNING]: Error handling client: Unix.Unix_error(Unix.EINVAL, "DRM_IOCTL_VIRTGPU_RESOURCE_CREATE_BLOB", "")
>> >
>> > I'm pretty confused by what this change was supposed to do in the first
>> > place…  Looking at how virtio_get_shm_region() is used in
>> > virtio_gpu_init(), it's called with a pointer to zeroed memory, and then
>> > the get_shm_region() implementation is supposed to write to the region,
>> > without ever reading from it as far as I can tell.  Why is the initial
>> > value of an out parameter being checked at all?  How does this prevent
>> > using zero-length shared memory regions?
>> >
>> > [1]: https://crosvm.dev/
>> > [2]: https://github.com/talex5/wayland-proxy-virtwl
>> >
>> > #regzbot introduced: 206cc44588f72b49ad4d7e21a7472ab2a72a83df
>> 
>> Okay, just found that it's already been reverted:
>> https://lore.kernel.org/all/20250808072533-mutt-send-email-mst@kernel.org/
>> 
>> Still, I'm confused how this was supposed to fix anything…
>> 
>> #regzbot fix: Revert "virtio: reject shm region if length is zero"
>
>
>
> Are you asking why was the patch applied in the 1st place?
> It seemed like an invalid behaviour to me, and I thought it's
> not too late to block it so we don't need to support it
> down the road.

So you just weren't aware during the review that it's an output
parameter rather than an input?  Should the parameter maybe be renamed
or something to make that more obvious?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

Re: [REGRESSION] virtio: reject shm region if length is zero

[Michael S. Tsirkin]

On Sat, Aug 16, 2025 at 01:05:21PM +0200, Alyssa Ross wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Fri, Aug 15, 2025 at 09:19:34PM +0200, Alyssa Ross wrote:
> >> Alyssa Ross <hi@alyssa.is> writes:
> >> 
> >> > On Mon, May 12, 2025 at 07:51:53AM +0930, SamiUddinsami.md.ko@gmail.com wrote:
> >> >> From: Sami Uddin <sami.md.ko@gmail.com>
> >> >>
> >> >> Prevent usage of shared memory regions where the length is zero,
> >> >> as such configurations are not valid and may lead to unexpected behavior.
> >> >>
> >> >> Signed-off-by: Sami Uddin <sami.md.ko@gmail.com>
> >> >> ---
> >> >> v3:
> >> >> - Use idiomatic 'if (!region->len)' as suggested by reviewer
> >> >> v2:
> >> >> - Fixed coding style issue: added space after 'if' statement
> >> >>
> >> >>  include/linux/virtio_config.h | 2 ++
> >> >>  1 file changed, 2 insertions(+)
> >> >
> >> > Hi, I'm sorry to be the bearer of bad news, but since this patch my VM
> >> > no longer works.  The system is running wayland-proxy-virtwl[1] inside
> >> > a crosvm[2] VM, using crosvm's virtio-gpu device to do cross-domain
> >> > Wayland forwarding.
> >> >
> >> > Since this change, wayland-proxy-virtwl crashes with the following log
> >> > message:
> >> >
> >> > 	wl-proxy [WARNING]: Error handling client: Unix.Unix_error(Unix.EINVAL, "DRM_IOCTL_VIRTGPU_RESOURCE_CREATE_BLOB", "")
> >> >
> >> > I'm pretty confused by what this change was supposed to do in the first
> >> > place…  Looking at how virtio_get_shm_region() is used in
> >> > virtio_gpu_init(), it's called with a pointer to zeroed memory, and then
> >> > the get_shm_region() implementation is supposed to write to the region,
> >> > without ever reading from it as far as I can tell.  Why is the initial
> >> > value of an out parameter being checked at all?  How does this prevent
> >> > using zero-length shared memory regions?
> >> >
> >> > [1]: https://crosvm.dev/
> >> > [2]: https://github.com/talex5/wayland-proxy-virtwl
> >> >
> >> > #regzbot introduced: 206cc44588f72b49ad4d7e21a7472ab2a72a83df
> >> 
> >> Okay, just found that it's already been reverted:
> >> https://lore.kernel.org/all/20250808072533-mutt-send-email-mst@kernel.org/
> >> 
> >> Still, I'm confused how this was supposed to fix anything…
> >> 
> >> #regzbot fix: Revert "virtio: reject shm region if length is zero"
> >
> >
> >
> > Are you asking why was the patch applied in the 1st place?
> > It seemed like an invalid behaviour to me, and I thought it's
> > not too late to block it so we don't need to support it
> > down the road.
> 
> So you just weren't aware during the review that it's an output
> parameter rather than an input?  Should the parameter maybe be renamed
> or something to make that more obvious?

Sounds like a good idea.

-- 
MST