| 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: <bdcb84be-75b6-4afb-91a7-40889fbb69fd@igalia.com>
Date: Sat, 13 Jul 2024 11:45:16 -0300
From: Maíra Canal <mcanal@...lia.com>
To: Louis Chauvet <louis.chauvet@...tlin.com>,
Rodrigo Siqueira <rodrigosiqueiramelo@...il.com>,
Melissa Wen <melissa.srw@...il.com>,
Haneen Mohammed <hamohammed.sa@...il.com>, Daniel Vetter <daniel@...ll.ch>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, rdunlap@...radead.org,
arthurgrillo@...eup.net, Jonathan Corbet <corbet@....net>,
pekka.paalanen@...oniitty.fi
Cc: dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
jeremie.dautheribes@...tlin.com, miquel.raynal@...tlin.com,
thomas.petazzoni@...tlin.com, seanpaul@...gle.com, marcheu@...gle.com,
nicolejadeyee@...gle.com, Pekka Paalanen <pekka.paalanen@...labora.com>
Subject: Re: [PATCH v8 05/17] drm/vkms: Add dummy pixel_read/pixel_write
callbacks to avoid NULL pointers
On 5/16/24 10:04, Louis Chauvet wrote:
> Introduce two callbacks which does nothing. They are used in replacement
> of NULL and it avoid kernel OOPS if this NULL is called.
I don't believe we should avoid a reasonable kernel OOPS. As you
noticed, if the user got this kernel OOPS it means that there is a
mismatch between what formats are announced by atomic_check and what is
really supported by atomic_update. This is a driver error.
If this happens, I want the kernel OOPS because it means I need to fix
the driver. Sometimes a kernel OOPS can save you some valuable debugging
time.
I'd probably drop this patch.
Best Regards,
- Maíra
>
> If those callback are used, it means that there is a mismatch between
> what formats are announced by atomic_check and what is realy supported by
> atomic_update.
>
> Acked-by: Pekka Paalanen <pekka.paalanen@...labora.com>
> Signed-off-by: Louis Chauvet <louis.chauvet@...tlin.com>
> ---
> drivers/gpu/drm/vkms/vkms_formats.c | 45 ++++++++++++++++++++++++++++++++-----
> 1 file changed, 39 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index 6b3e17374b19..c28c32b00e39 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -135,6 +135,21 @@ static void RGB565_to_argb_u16(u8 *in_pixel, struct pixel_argb_u16 *out_pixel)
> out_pixel->b = drm_fixp2int_round(drm_fixp_mul(fp_b, fp_rb_ratio));
> }
>
> +/**
> + * magenta_to_argb_u16() - pixel_read callback which always read magenta
> + *
> + * This callback is used when an invalid format is requested for plane reading.
> + * It is used to avoid null pointer to be used as a function. In theory, this function should
> + * never be called, except if you found a bug in the driver/DRM core.
> + */
> +static void magenta_to_argb_u16(u8 *in_pixel, struct pixel_argb_u16 *out_pixel)
> +{
> + out_pixel->a = (u16)0xFFFF;
> + out_pixel->r = (u16)0xFFFF;
> + out_pixel->g = 0;
> + out_pixel->b = (u16)0xFFFF;
> +}
> +
> /**
> * vkms_compose_row - compose a single row of a plane
> * @stage_buffer: output line with the composed pixels
> @@ -237,6 +252,16 @@ static void argb_u16_to_RGB565(u8 *out_pixel, struct pixel_argb_u16 *in_pixel)
> *pixel = cpu_to_le16(r << 11 | g << 5 | b);
> }
>
> +/**
> + * argb_u16_to_nothing() - pixel_write callback with no effect
> + *
> + * This callback is used when an invalid format is requested for writeback.
> + * It is used to avoid null pointer to be used as a function. In theory, this should never
> + * happen, except if there is a bug in the driver
> + */
> +static void argb_u16_to_nothing(u8 *out_pixel, struct pixel_argb_u16 *in_pixel)
> +{}
> +
> /**
> * vkms_writeback_row() - Generic loop for all supported writeback format. It is executed just
> * after the blending to write a line in the writeback buffer.
> @@ -260,8 +285,10 @@ void vkms_writeback_row(struct vkms_writeback_job *wb,
>
> /**
> * get_pixel_conversion_function() - Retrieve the correct read_pixel function for a specific
> - * format. The returned pointer is NULL for unsupported pixel formats. The caller must ensure that
> - * the pointer is valid before using it in a vkms_plane_state.
> + * format.
> + *
> + * If the format is not supported by VKMS a warning is emitted and a dummy "always read magenta"
> + * function is returned.
> *
> * @format: DRM_FORMAT_* value for which to obtain a conversion function (see [drm_fourcc.h])
> */
> @@ -284,18 +311,21 @@ pixel_read_t get_pixel_read_function(u32 format)
> * format must:
> * - Be listed in vkms_formats in vkms_plane.c
> * - Have a pixel_read callback defined here
> + *
> + * To avoid kernel crash, a dummy "always read magenta" function is used. It means
> + * that during the composition, this plane will always be magenta.
> */
> WARN(true,
> "Pixel format %p4cc is not supported by VKMS planes. This is a kernel bug, atomic check must forbid this configuration.\n",
> &format);
> - return (pixel_read_t)NULL;
> + return &magenta_to_argb_u16;
> }
> }
>
> /**
> * get_pixel_write_function() - Retrieve the correct write_pixel function for a specific format.
> - * The returned pointer is NULL for unsupported pixel formats. The caller must ensure that the
> - * pointer is valid before using it in a vkms_writeback_job.
> + * If the format is not supported by VKMS a warning is emitted and a dummy "don't do anything"
> + * function is returned.
> *
> * @format: DRM_FORMAT_* value for which to obtain a conversion function (see [drm_fourcc.h])
> */
> @@ -318,10 +348,13 @@ pixel_write_t get_pixel_write_function(u32 format)
> * format must:
> * - Be listed in vkms_wb_formats in vkms_writeback.c
> * - Have a pixel_write callback defined here
> + *
> + * To avoid kernel crash, a dummy "don't do anything" function is used. It means
> + * that the resulting writeback buffer is not composed and can contains any values.
> */
> WARN(true,
> "Pixel format %p4cc is not supported by VKMS writeback. This is a kernel bug, atomic check must forbid this configuration.\n",
> &format);
> - return (pixel_write_t)NULL;
> + return &argb_u16_to_nothing;
> }
> }
>
Powered by blists - more mailing lists