linux-kernel - RE: [PATCH] drm/vkms: use bitfield op to get xrgb on compute crc

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 23 Mar 2020 14:54:40 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Melissa Wen' <melissa.srw@...il.com>,
        Rodrigo Siqueira <rodrigosiqueiramelo@...il.com>,
        Haneen Mohammed <hamohammed.sa@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>,
        "Rodrigo.Siqueira@....com" <Rodrigo.Siqueira@....com>
CC:     "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] drm/vkms: use bitfield op to get xrgb on compute crc

From: Melissa Wen
> Sent: 21 March 2020 20:37
> The previous memset operation was not correctly setting zero on the
> alpha channel to compute the crc, and as a result, the
> pipe-A-cursor-alpha-transparent subtest of the IGT test kms_cursor_crc
> was crashing. To avoid errors of misinterpretation related to
> endianness, this solution uses a bitfield operation to extract the RGB
> values from each pixel and ignores the alpha channel as expected.
> 
> Signed-off-by: Melissa Wen <melissa.srw@...il.com>
> ---
>  drivers/gpu/drm/vkms/vkms_composer.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 4af2f19480f4..8c1c005bb717 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0+
> 
>  #include <linux/crc32.h>
> +#include <linux/bitfield.h>
> 
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
> @@ -9,6 +10,8 @@
> 
>  #include "vkms_drv.h"
> 
> +#define XRGB_MSK GENMASK(23, 0)
> +
>  /**
>   * compute_crc - Compute CRC value on output frame
>   *
> @@ -26,6 +29,7 @@ static uint32_t compute_crc(void *vaddr_out, struct vkms_composer *composer)
>  	int h_src = drm_rect_height(&composer->src) >> 16;
>  	int w_src = drm_rect_width(&composer->src) >> 16;
>  	u32 crc = 0;
> +	u32 *pixel;
> 
>  	for (i = y_src; i < y_src + h_src; ++i) {
>  		for (j = x_src; j < x_src + w_src; ++j) {
> @@ -33,7 +37,8 @@ static uint32_t compute_crc(void *vaddr_out, struct vkms_composer *composer)
>  				     + (i * composer->pitch)
>  				     + (j * composer->cpp);
>  			/* XRGB format ignores Alpha channel */
> -			memset(vaddr_out + src_offset + 24, 0,  8);
> +			pixel = vaddr_out + src_offset;
> +			*pixel = FIELD_GET(XRGB_MSK, *(u32 *)pixel);
>  			crc = crc32_le(crc, vaddr_out + src_offset,
>  				       sizeof(u32));

That looks horrid.
I suspect the simplest fix is to change the memset() offset/length
to bytes from bits.
Or (assuming the alpha channel is last) just:
	*(u8 *)(vaddr_out + src_offset + 3) = 0;
I'm not sure of the options for the crc code, but if you are only
passing in 4 bytes there ought to be an option to pass in the value
itself (rather than the address).

Do you actually want to 'zap' the alpha channel data from the
output buffer? or just exclude it from the crc??

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)