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

Open Source and information security mailing list archives

Date:   Tue, 16 Jul 2019 10:37:15 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Rodrigo Siqueira <rodrigosiqueiramelo@...il.com>
Cc:     Brian Starkey <brian.starkey@....com>,
        Liviu Dudau <liviu.dudau@....com>,
        Haneen Mohammed <hamohammed.sa@...il.com>,
        Simon Ser <contact@...rsion.fr>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V3 4/5] drm/vkms: Compute CRC without change input data

On Fri, Jul 12, 2019 at 12:14:49AM -0300, Rodrigo Siqueira wrote:
> On 07/11, Daniel Vetter wrote:
> > On Tue, Jun 25, 2019 at 10:38:31PM -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.
> > 
> > Uh why? For writeback we're writing the output too, so we can write
> > whatever we want to into the alpha channel. For writeback we should never
> > accept a pixel format where alpha actually matters, that doesn't make
> > sense. You can't see through a real screen either, they are all opaque :-)
> > -Daniel
> 
> Hmmm,
> 
> I see your point and I agree, but even though we can write whatever we
> want in the output, don’t you think that is weird to change the
> framebuffer value in the compute_crc() function?

Not sure what you mean here ... ? From a quick look the memset only sets
our temporary buffer, so we're not changing the input framebuffer here.
And we have to somehow get rid of the X bits, since there's no alpha
value. For CRC computation, all we need is some value which is the same
for every frame (so that the CRC stays constant for the same visible
output). For writeback we could write whatever we want (which includes
whatever is there already). But there's no guarantee and definitely no
expectation that the X bits survive. Writing 0 is imo the most reasonable
thing to do. I'm not even sure whether modern gpus can still do channel
masking (i.e. only write out specific channels, instead of the entire
color). That was a "feature" of bitop blitters of the 80s/90s :-)
-Daniel

> 
> Thanks
>  
> > > 
> > > 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);
> > > -			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));
> > >  		}
> > >  	}
> > >  
> > > -- 
> > > 2.21.0
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Rodrigo Siqueira
> https://siqueira.tech


> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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