[RFC,v2,05/17] drm/vkms: Avoid reading beyond LUT array

Commit Message

Harry Wentland Oct. 19, 2023, 9:21 p.m. UTC

When the floor LUT index (drm_fixp2int(lut_index) is the last
index of the array the ceil LUT index will point to an entry
beyond the array. Make sure we guard against it and use the
value of the floot LUT index.

Blurb about LUT creation and how first element should be 0x0 and
last one 0xffff.

Hold on, is that even correct? What should the ends of a LUT be?
How does UNORM work and how does it apply to LUTs?

Signed-off-by: Harry Wentland <[e-mail]>
Cc: Ville Syrjala <[e-mail]>
Cc: Pekka Paalanen <[e-mail]>
Cc: Simon Ser <[e-mail]>
Cc: Harry Wentland <[e-mail]>
Cc: Melissa Wen <[e-mail]>
Cc: Jonas Ådahl <[e-mail]>
Cc: Sebastian Wick <[e-mail]>
Cc: Shashank Sharma <[e-mail]>
Cc: Alexander Goins <[e-mail]>
Cc: Joshua Ashton <[e-mail]>
Cc: Michel Dänzer <[e-mail]>
Cc: Aleix Pol <[e-mail]>
Cc: Xaver Hugl <[e-mail]>
Cc: Victoria Brekenfeld <[e-mail]>
Cc: Sima <[e-mail]>
Cc: Uma Shankar <[e-mail]>
Cc: Naseer Ahmed <[e-mail]>
Cc: Christopher Braga <[e-mail]>
Cc: Abhinav Kumar <[e-mail]>
Cc: Arthur Grillo <[e-mail]>
Cc: Hector Martin <[e-mail]>
Cc: Liviu Dudau <[e-mail]>
Cc: Sasha McIntosh <[e-mail]>
---
 drivers/gpu/drm/vkms/vkms_composer.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

Comments

Pekka Paalanen Oct. 30, 2023, 1:29 p.m. UTC | #1

On Thu, 19 Oct 2023 17:21:21 -0400
Harry Wentland <[e-mail]> wrote:

> When the floor LUT index (drm_fixp2int(lut_index) is the last
> index of the array the ceil LUT index will point to an entry
> beyond the array. Make sure we guard against it and use the
> value of the floot LUT index.
> 
> Blurb about LUT creation and how first element should be 0x0 and
> last one 0xffff.
> 
> Hold on, is that even correct? What should the ends of a LUT be?
> How does UNORM work and how does it apply to LUTs?

Do you mean how should UNORM input value map to LUT entries for LUT
indexing?

I suppose UNORM 16-bit converts to nominal real values as:
- 0x0: 0.0
- 0xffff: 1.0

And in LUT, you want 0.0 to map to the first LUT element exactly, and
1.0 to map to the last LUT element exactly, even if whatever
interpolation may be in use, right?

If so, it is important to make sure that, assuming linear interpolation
for instance, there is no "dead zone" at either end. Given high
interpolation precision, any step away from 0.0 or 1.0 needs to imply a
change in the real-valued output, assuming e.g. identity LUT.

If LUT has N elements, and 16-bit UNORM input value is I, then (in
naive real-valued math, so no implicit truncation between operations)

x = I / 0xffff * (N - 1)
ia = floor(x)
ib = min(ia + 1, N - 1)

f = x - floor(x)
y = (1 - f) * LUT[ia] + f * LUT[ib]


Does that help?

In my mind, I'm thinking of a uniformly distributed LUT as a 1-D
texture, because that's how I have implemented them in GL. There you
have to be careful so that input values 0.0 and 1.0 map to the *center*
of the first and last texel, and not to the edges of the texture like
texture coordinates do. Then you can use the GL linear texture
interpolation as-is.


Thanks,
pq


> Signed-off-by: Harry Wentland <[e-mail]>
> Cc: Ville Syrjala <[e-mail]>
> Cc: Pekka Paalanen <[e-mail]>
> Cc: Simon Ser <[e-mail]>
> Cc: Harry Wentland <[e-mail]>
> Cc: Melissa Wen <[e-mail]>
> Cc: Jonas Ådahl <[e-mail]>
> Cc: Sebastian Wick <[e-mail]>
> Cc: Shashank Sharma <[e-mail]>
> Cc: Alexander Goins <[e-mail]>
> Cc: Joshua Ashton <[e-mail]>
> Cc: Michel Dänzer <[e-mail]>
> Cc: Aleix Pol <[e-mail]>
> Cc: Xaver Hugl <[e-mail]>
> Cc: Victoria Brekenfeld <[e-mail]>
> Cc: Sima <[e-mail]>
> Cc: Uma Shankar <[e-mail]>
> Cc: Naseer Ahmed <[e-mail]>
> Cc: Christopher Braga <[e-mail]>
> Cc: Abhinav Kumar <[e-mail]>
> Cc: Arthur Grillo <[e-mail]>
> Cc: Hector Martin <[e-mail]>
> Cc: Liviu Dudau <[e-mail]>
> Cc: Sasha McIntosh <[e-mail]>
> ---
>  drivers/gpu/drm/vkms/vkms_composer.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index a0a3a6fd2926..cf1dff162920 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -123,6 +123,8 @@ static u16 apply_lut_to_channel_value(const struct vkms_color_lut *lut, u16 chan
>  				      enum lut_channel channel)
>  {
>  	s64 lut_index = get_lut_index(lut, channel_value);
> +	u16 *floor_lut_value, *ceil_lut_value;
> +	u16 floor_channel_value, ceil_channel_value;
>  
>  	/*
>  	 * This checks if `struct drm_color_lut` has any gap added by the compiler
> @@ -130,11 +132,15 @@ static u16 apply_lut_to_channel_value(const struct vkms_color_lut *lut, u16 chan
>  	 */
>  	static_assert(sizeof(struct drm_color_lut) == sizeof(__u16) * 4);
>  
> -	u16 *floor_lut_value = (__u16 *)&lut->base[drm_fixp2int(lut_index)];
> -	u16 *ceil_lut_value = (__u16 *)&lut->base[drm_fixp2int_ceil(lut_index)];
> +	floor_lut_value = (__u16 *)&lut->base[drm_fixp2int(lut_index)];
> +	if (drm_fixp2int(lut_index) == (lut->lut_length - 1))
> +		/* We're at the end of the LUT array, use same value for ceil and floor */
> +		ceil_lut_value = floor_lut_value;
> +	else
> +		ceil_lut_value = (__u16 *)&lut->base[drm_fixp2int_ceil(lut_index)];
>  
> -	u16 floor_channel_value = floor_lut_value[channel];
> -	u16 ceil_channel_value = ceil_lut_value[channel];
> +	floor_channel_value = floor_lut_value[channel];
> +	ceil_channel_value = ceil_lut_value[channel];
>  
>  	return lerp_u16(floor_channel_value, ceil_channel_value,
>  			lut_index & DRM_FIXED_DECIMAL_MASK);

Harry Wentland Nov. 6, 2023, 8:48 p.m. UTC | #2

On 2023-10-30 09:29, Pekka Paalanen wrote:
> On Thu, 19 Oct 2023 17:21:21 -0400
> Harry Wentland <[e-mail]> wrote:
> 
>> When the floor LUT index (drm_fixp2int(lut_index) is the last
>> index of the array the ceil LUT index will point to an entry
>> beyond the array. Make sure we guard against it and use the
>> value of the floot LUT index.
>>
>> Blurb about LUT creation and how first element should be 0x0 and
>> last one 0xffff.
>>
>> Hold on, is that even correct? What should the ends of a LUT be?
>> How does UNORM work and how does it apply to LUTs?
> 
> Do you mean how should UNORM input value map to LUT entries for LUT
> indexing?
> 
> I suppose UNORM 16-bit converts to nominal real values as:
> - 0x0: 0.0
> - 0xffff: 1.0
> 
> And in LUT, you want 0.0 to map to the first LUT element exactly, and
> 1.0 to map to the last LUT element exactly, even if whatever
> interpolation may be in use, right?
> 
> If so, it is important to make sure that, assuming linear interpolation
> for instance, there is no "dead zone" at either end. Given high
> interpolation precision, any step away from 0.0 or 1.0 needs to imply a
> change in the real-valued output, assuming e.g. identity LUT.
> 
> If LUT has N elements, and 16-bit UNORM input value is I, then (in
> naive real-valued math, so no implicit truncation between operations)
> 
> x = I / 0xffff * (N - 1)
> ia = floor(x)
> ib = min(ia + 1, N - 1)
> 
> f = x - floor(x)
> y = (1 - f) * LUT[ia] + f * LUT[ib]
> 
> 
> Does that help?
> 

Thanks. Yes, this is what the code is doing (with this commit).

The commit description was an oversight and only reflect my initial
thoughts when coding it, before I made sure this is the right way
to go about it. I'll update it.

Harry

> In my mind, I'm thinking of a uniformly distributed LUT as a 1-D
> texture, because that's how I have implemented them in GL. There you
> have to be careful so that input values 0.0 and 1.0 map to the *center*
> of the first and last texel, and not to the edges of the texture like
> texture coordinates do. Then you can use the GL linear texture
> interpolation as-is.
> 
> 
> Thanks,
> pq
> 
> 
>> Signed-off-by: Harry Wentland <[e-mail]>
>> Cc: Ville Syrjala <[e-mail]>
>> Cc: Pekka Paalanen <[e-mail]>
>> Cc: Simon Ser <[e-mail]>
>> Cc: Harry Wentland <[e-mail]>
>> Cc: Melissa Wen <[e-mail]>
>> Cc: Jonas Ådahl <[e-mail]>
>> Cc: Sebastian Wick <[e-mail]>
>> Cc: Shashank Sharma <[e-mail]>
>> Cc: Alexander Goins <[e-mail]>
>> Cc: Joshua Ashton <[e-mail]>
>> Cc: Michel Dänzer <[e-mail]>
>> Cc: Aleix Pol <[e-mail]>
>> Cc: Xaver Hugl <[e-mail]>
>> Cc: Victoria Brekenfeld <[e-mail]>
>> Cc: Sima <[e-mail]>
>> Cc: Uma Shankar <[e-mail]>
>> Cc: Naseer Ahmed <[e-mail]>
>> Cc: Christopher Braga <[e-mail]>
>> Cc: Abhinav Kumar <[e-mail]>
>> Cc: Arthur Grillo <[e-mail]>
>> Cc: Hector Martin <[e-mail]>
>> Cc: Liviu Dudau <[e-mail]>
>> Cc: Sasha McIntosh <[e-mail]>
>> ---
>>  drivers/gpu/drm/vkms/vkms_composer.c | 14 ++++++++++----
>>  1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
>> index a0a3a6fd2926..cf1dff162920 100644
>> --- a/drivers/gpu/drm/vkms/vkms_composer.c
>> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
>> @@ -123,6 +123,8 @@ static u16 apply_lut_to_channel_value(const struct vkms_color_lut *lut, u16 chan
>>  				      enum lut_channel channel)
>>  {
>>  	s64 lut_index = get_lut_index(lut, channel_value);
>> +	u16 *floor_lut_value, *ceil_lut_value;
>> +	u16 floor_channel_value, ceil_channel_value;
>>  
>>  	/*
>>  	 * This checks if `struct drm_color_lut` has any gap added by the compiler
>> @@ -130,11 +132,15 @@ static u16 apply_lut_to_channel_value(const struct vkms_color_lut *lut, u16 chan
>>  	 */
>>  	static_assert(sizeof(struct drm_color_lut) == sizeof(__u16) * 4);
>>  
>> -	u16 *floor_lut_value = (__u16 *)&lut->base[drm_fixp2int(lut_index)];
>> -	u16 *ceil_lut_value = (__u16 *)&lut->base[drm_fixp2int_ceil(lut_index)];
>> +	floor_lut_value = (__u16 *)&lut->base[drm_fixp2int(lut_index)];
>> +	if (drm_fixp2int(lut_index) == (lut->lut_length - 1))
>> +		/* We're at the end of the LUT array, use same value for ceil and floor */
>> +		ceil_lut_value = floor_lut_value;
>> +	else
>> +		ceil_lut_value = (__u16 *)&lut->base[drm_fixp2int_ceil(lut_index)];
>>  
>> -	u16 floor_channel_value = floor_lut_value[channel];
>> -	u16 ceil_channel_value = ceil_lut_value[channel];
>> +	floor_channel_value = floor_lut_value[channel];
>> +	ceil_channel_value = ceil_lut_value[channel];
>>  
>>  	return lerp_u16(floor_channel_value, ceil_channel_value,
>>  			lut_index & DRM_FIXED_DECIMAL_MASK);
>

Patch

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index a0a3a6fd2926..cf1dff162920 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -123,6 +123,8 @@  static u16 apply_lut_to_channel_value(const struct vkms_color_lut *lut, u16 chan
 				      enum lut_channel channel)
 {
 	s64 lut_index = get_lut_index(lut, channel_value);
+	u16 *floor_lut_value, *ceil_lut_value;
+	u16 floor_channel_value, ceil_channel_value;
 
 	/*
 	 * This checks if `struct drm_color_lut` has any gap added by the compiler
@@ -130,11 +132,15 @@  static u16 apply_lut_to_channel_value(const struct vkms_color_lut *lut, u16 chan
 	 */
 	static_assert(sizeof(struct drm_color_lut) == sizeof(__u16) * 4);
 
-	u16 *floor_lut_value = (__u16 *)&lut->base[drm_fixp2int(lut_index)];
-	u16 *ceil_lut_value = (__u16 *)&lut->base[drm_fixp2int_ceil(lut_index)];
+	floor_lut_value = (__u16 *)&lut->base[drm_fixp2int(lut_index)];
+	if (drm_fixp2int(lut_index) == (lut->lut_length - 1))
+		/* We're at the end of the LUT array, use same value for ceil and floor */
+		ceil_lut_value = floor_lut_value;
+	else
+		ceil_lut_value = (__u16 *)&lut->base[drm_fixp2int_ceil(lut_index)];
 
-	u16 floor_channel_value = floor_lut_value[channel];
-	u16 ceil_channel_value = ceil_lut_value[channel];
+	floor_channel_value = floor_lut_value[channel];
+	ceil_channel_value = ceil_lut_value[channel];
 
 	return lerp_u16(floor_channel_value, ceil_channel_value,
 			lut_index & DRM_FIXED_DECIMAL_MASK);