linux-kernel - Re: [PATCH] drm/ssd130x: Iterate over damage clips instead of using a merged rect

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]

Message-ID: <79e665bb-c51a-0ac1-3f2f-f0e5eba22388@redhat.com>
Date:   Fri, 30 Sep 2022 13:22:16 +0200
From:   Javier Martinez Canillas <javierm@...hat.com>
To:     Thomas Zimmermann <tzimmermann@...e.de>,
        linux-kernel@...r.kernel.org
Cc:     David Airlie <airlied@...ux.ie>,
        Jocelyn Falempe <jfalempe@...hat.com>,
        dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH] drm/ssd130x: Iterate over damage clips instead of using a
 merged rect

Hello Thomas,

On 9/30/22 12:11, Thomas Zimmermann wrote:

[...]

>>
>>>> +
>>>> +		ssd130x_fb_blit_rect(plane_state->fb, &shadow_plane_state->data[0], &damage);
>>>
>>> In simpledrm, we adjust the destination address with dst_clip like this:
>>>
>>>     iosys_map_incr(&dst, drm_fb_clip_offset(sdev->pitch, sdev->format,
>>> &dst_clip));
>>>
>>> How does this work in ssd130x? You never use dst_clip to adjust to the
>>> changed location. Won't you have out-of-bounds writes on the device?
>>>
>>
>> Right, in ssd130x what I do is:
>>
>> static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb, const struct iosys_map *vmap,
>> 				struct drm_rect *rect)
>> {
>> 	struct iosys_map dst;
>> ...
>> 	u8 *buf = NULL;
>> ...
>> 	buf = kcalloc(dst_pitch, drm_rect_height(rect), GFP_KERNEL);
>> ...
>> 	iosys_map_set_vaddr(&dst, buf);
>> 	drm_fb_xrgb8888_to_mono(&dst, &dst_pitch, vmap, fb, rect);
>> ...
>> 	ssd130x_update_rect(ssd130x, buf, rect);
>> }
>>
>> I understand that's correct too?
> 
>  From what I understand about ssd130x, blit_rect looks correct up to the 
> call to update_rect.  The values in the rect parameter are for the 
> damage area of the plane. In update_rect, the destination coords x and y 
> are also taken from rect. But they should come from dst_clip, which is 
> the on-screen location. Does that make sense?
> 

I believe you are correct. Then what I should do is to not pass the damage
area to ssd130x_fb_blit_rect() as the struct drm_rect argument but instead
the dst_clip as filled by drm_rect_intersect(). Does that sound correct ?

In other words, the following:

	drm_atomic_helper_damage_iter_init(&iter, old_plane_state, plane_state);
	drm_atomic_for_each_plane_damage(&iter, &damage) {
		dst_clip = plane_state->dst;
		if (!drm_rect_intersect(&dst_clip, &damage))
			continue;

		ssd130x_fb_blit_rect(plane_state->fb, &shadow_plane_state->data[0], &dst_clip);
	}

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat