[PATCH resend] drm/vboxvideo: Replace fake VLA at end of vbva_mouse_pointer_shape with real VLA

[PATCH resend] drm/vboxvideo: Replace fake VLA at end of vbva_mouse_pointer_shape with real VLA

[Hans de Goede]

Replace the fake VLA at end of the vbva_mouse_pointer_shape shape with
a real VLA to fix a "memcpy: detected field-spanning write error" warning:

[   13.319813] memcpy: detected field-spanning write (size 16896) of single field "p->data" at drivers/gpu/drm/vboxvideo/hgsmi_base.c:154 (size 4)
[   13.319841] WARNING: CPU: 0 PID: 1105 at drivers/gpu/drm/vboxvideo/hgsmi_base.c:154 hgsmi_update_pointer_shape+0x192/0x1c0 [vboxvideo]
[   13.320038] Call Trace:
[   13.320173]  hgsmi_update_pointer_shape [vboxvideo]
[   13.320184]  vbox_cursor_atomic_update [vboxvideo]

Note as mentioned in the added comment it seems the original length
calculation for the allocated and send hgsmi buffer is 4 bytes too large.
Changing this is not the goal of this patch, so this behavior is kept.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Resend, can I get an ack from someone to push this to drm-misc-next please ?

(this has been tested with a vboxsvga adapter in a vbox vm)
---
 drivers/gpu/drm/vboxvideo/hgsmi_base.c | 10 +++++++++-
 drivers/gpu/drm/vboxvideo/vboxvideo.h  |  4 +---
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/vboxvideo/hgsmi_base.c b/drivers/gpu/drm/vboxvideo/hgsmi_base.c
index 8c041d7ce4f1..87dccaecc3e5 100644
--- a/drivers/gpu/drm/vboxvideo/hgsmi_base.c
+++ b/drivers/gpu/drm/vboxvideo/hgsmi_base.c
@@ -139,7 +139,15 @@ int hgsmi_update_pointer_shape(struct gen_pool *ctx, u32 flags,
 		flags |= VBOX_MOUSE_POINTER_VISIBLE;
 	}
 
-	p = hgsmi_buffer_alloc(ctx, sizeof(*p) + pixel_len, HGSMI_CH_VBVA,
+	/*
+	 * The 4 extra bytes come from switching struct vbva_mouse_pointer_shape
+	 * from having a 4 bytes fixed array at the end to using a proper VLA
+	 * at the end. These 4 extra bytes were not subtracted from sizeof(*p)
+	 * before the switch to the VLA, so this way the behavior is unchanged.
+	 * Chances are these 4 extra bytes are not necessary but they are kept
+	 * to avoid regressions.
+	 */
+	p = hgsmi_buffer_alloc(ctx, sizeof(*p) + pixel_len + 4, HGSMI_CH_VBVA,
 			       VBVA_MOUSE_POINTER_SHAPE);
 	if (!p)
 		return -ENOMEM;
diff --git a/drivers/gpu/drm/vboxvideo/vboxvideo.h b/drivers/gpu/drm/vboxvideo/vboxvideo.h
index f60d82504da0..79ec8481de0e 100644
--- a/drivers/gpu/drm/vboxvideo/vboxvideo.h
+++ b/drivers/gpu/drm/vboxvideo/vboxvideo.h
@@ -351,10 +351,8 @@ struct vbva_mouse_pointer_shape {
 	 * Bytes in the gap between the AND and the XOR mask are undefined.
 	 * XOR mask scanlines have no gap between them and size of XOR mask is:
 	 * xor_len = width * 4 * height.
-	 *
-	 * Preallocate 4 bytes for accessing actual data as p->data.
 	 */
-	u8 data[4];
+	u8 data[];
 } __packed;
 
 /* pointer is visible */
-- 
2.46.0

Re: [PATCH resend] drm/vboxvideo: Replace fake VLA at end of vbva_mouse_pointer_shape with real VLA

[Jani Nikula]

On Tue, 27 Aug 2024, Hans de Goede <hdegoede@redhat.com> wrote:
> Replace the fake VLA at end of the vbva_mouse_pointer_shape shape with
> a real VLA to fix a "memcpy: detected field-spanning write error" warning:
>
> [   13.319813] memcpy: detected field-spanning write (size 16896) of single field "p->data" at drivers/gpu/drm/vboxvideo/hgsmi_base.c:154 (size 4)
> [   13.319841] WARNING: CPU: 0 PID: 1105 at drivers/gpu/drm/vboxvideo/hgsmi_base.c:154 hgsmi_update_pointer_shape+0x192/0x1c0 [vboxvideo]
> [   13.320038] Call Trace:
> [   13.320173]  hgsmi_update_pointer_shape [vboxvideo]
> [   13.320184]  vbox_cursor_atomic_update [vboxvideo]
>
> Note as mentioned in the added comment it seems the original length
> calculation for the allocated and send hgsmi buffer is 4 bytes too large.
> Changing this is not the goal of this patch, so this behavior is kept.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Resend, can I get an ack from someone to push this to drm-misc-next please ?
>
> (this has been tested with a vboxsvga adapter in a vbox vm)
> ---
>  drivers/gpu/drm/vboxvideo/hgsmi_base.c | 10 +++++++++-
>  drivers/gpu/drm/vboxvideo/vboxvideo.h  |  4 +---
>  2 files changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/vboxvideo/hgsmi_base.c b/drivers/gpu/drm/vboxvideo/hgsmi_base.c
> index 8c041d7ce4f1..87dccaecc3e5 100644
> --- a/drivers/gpu/drm/vboxvideo/hgsmi_base.c
> +++ b/drivers/gpu/drm/vboxvideo/hgsmi_base.c
> @@ -139,7 +139,15 @@ int hgsmi_update_pointer_shape(struct gen_pool *ctx, u32 flags,
>  		flags |= VBOX_MOUSE_POINTER_VISIBLE;
>  	}
>  
> -	p = hgsmi_buffer_alloc(ctx, sizeof(*p) + pixel_len, HGSMI_CH_VBVA,
> +	/*
> +	 * The 4 extra bytes come from switching struct vbva_mouse_pointer_shape
> +	 * from having a 4 bytes fixed array at the end to using a proper VLA
> +	 * at the end. These 4 extra bytes were not subtracted from sizeof(*p)
> +	 * before the switch to the VLA, so this way the behavior is unchanged.
> +	 * Chances are these 4 extra bytes are not necessary but they are kept
> +	 * to avoid regressions.
> +	 */
> +	p = hgsmi_buffer_alloc(ctx, sizeof(*p) + pixel_len + 4, HGSMI_CH_VBVA,

Nitpick, struct_size(p, data, pixel_len + 4) is a better choice for VLAs
than open coding.

Either way, and FWIW,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


BR,
Jani.


>  			       VBVA_MOUSE_POINTER_SHAPE);
>  	if (!p)
>  		return -ENOMEM;
> diff --git a/drivers/gpu/drm/vboxvideo/vboxvideo.h b/drivers/gpu/drm/vboxvideo/vboxvideo.h
> index f60d82504da0..79ec8481de0e 100644
> --- a/drivers/gpu/drm/vboxvideo/vboxvideo.h
> +++ b/drivers/gpu/drm/vboxvideo/vboxvideo.h
> @@ -351,10 +351,8 @@ struct vbva_mouse_pointer_shape {
>  	 * Bytes in the gap between the AND and the XOR mask are undefined.
>  	 * XOR mask scanlines have no gap between them and size of XOR mask is:
>  	 * xor_len = width * 4 * height.
> -	 *
> -	 * Preallocate 4 bytes for accessing actual data as p->data.
>  	 */
> -	u8 data[4];
> +	u8 data[];
>  } __packed;
>  
>  /* pointer is visible */

-- 
Jani Nikula, Intel