linux-kernel - Re: [PATCH V3 4/5] drm/vkms: Compute CRC without change input data

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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 9 Jul 2019 10:05:22 +0000
From:   "Vasilev, Oleg" <oleg.vasilev@...el.com>
To:     "liviu.dudau@....com" <liviu.dudau@....com>,
        "daniel@...ll.ch" <daniel@...ll.ch>,
        "brian.starkey@....com" <brian.starkey@....com>,
        "hamohammed.sa@...il.com" <hamohammed.sa@...il.com>,
        "rodrigosiqueiramelo@...il.com" <rodrigosiqueiramelo@...il.com>,
        "contact@...rsion.fr" <contact@...rsion.fr>
CC:     "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V3 4/5] drm/vkms: Compute CRC without change input data

On Tue, 2019-06-25 at 22:38 -0300, Rodrigo Siqueira wrote:
> The compute_crc() function is responsible for calculating the
> framebuffer CRC value; due to the XRGB format, this function has to
> ignore the alpha channel during the CRC computation. Therefore,
> compute_crc() set zero to the alpha channel directly in the input
> framebuffer, which is not a problem since this function receives a
> copy
> of the original buffer. However, if we want to use this function in a
> context without a buffer copy, it will change the initial value. This
> patch makes compute_crc() calculate the CRC value without modifying
> the
> input framebuffer.
> 
> Signed-off-by: Rodrigo Siqueira <rodrigosiqueiramelo@...il.com>
> ---
>  drivers/gpu/drm/vkms/vkms_composer.c | 31 +++++++++++++++++---------
> --
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c
> b/drivers/gpu/drm/vkms/vkms_composer.c
> index 51a270514219..8126aa0f968f 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -6,33 +6,40 @@
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
>  
> +static u32 get_pixel_from_buffer(int x, int y, const u8 *buffer,
> +				 const struct vkms_composer *composer)
> +{
> +	int src_offset = composer->offset + (y * composer->pitch)
> +					  + (x * composer->cpp);
> +
> +	return *(u32 *)&buffer[src_offset];
> +}
> +
>  /**
>   * compute_crc - Compute CRC value on output frame
>   *
> - * @vaddr_out: address to final framebuffer
> + * @vaddr: address to final framebuffer
>   * @composer: framebuffer's metadata
>   *
>   * returns CRC value computed using crc32 on the visible portion of
>   * the final framebuffer at vaddr_out
>   */
> -static uint32_t compute_crc(void *vaddr_out, struct vkms_composer
> *composer)
> +static uint32_t compute_crc(const u8 *vaddr,
> +			    const struct vkms_composer *composer)
>  {
> -	int i, j, src_offset;
> +	int x, y;
>  	int x_src = composer->src.x1 >> 16;
>  	int y_src = composer->src.y1 >> 16;
>  	int h_src = drm_rect_height(&composer->src) >> 16;
>  	int w_src = drm_rect_width(&composer->src) >> 16;
> -	u32 crc = 0;
> +	u32 crc = 0, pixel = 0;
>  
> -	for (i = y_src; i < y_src + h_src; ++i) {
> -		for (j = x_src; j < x_src + w_src; ++j) {
> -			src_offset = composer->offset
> -				     + (i * composer->pitch)
> -				     + (j * composer->cpp);
> +	for (y = y_src; y < y_src + h_src; ++y) {
> +		for (x = x_src; x < x_src + w_src; ++x) {
>  			/* XRGB format ignores Alpha channel */
> -			memset(vaddr_out + src_offset + 24, 0,  8);
Hi Rodgrigo,

Do I understand correctly, that previous version with memset was
actually zeroing out the whole fb, except first 24 bytes? On the first
iteration bytes 24..32 would be zeroed, on the second 25..33, etc.

Should we add more CRC tests to IGT, so we can catch such mistakes? For
example, compute CRC, than augment random pixel and assert that CRC
changed.

Oleg
> -			crc = crc32_le(crc, vaddr_out + src_offset,
> -				       sizeof(u32));
> +			pixel = get_pixel_from_buffer(x, y, vaddr,
> composer);
> +			bitmap_clear((void *)&pixel, 0, 8);
> +			crc = crc32_le(crc, (void *)&pixel,
> sizeof(u32));
>  		}
>  	}
>  

Download attachment "smime.p7s" of type "application/x-pkcs7-signature" (3261 bytes)