[PATCH] drm/sun4i: dsi: Prevent underflow when computing packet sizes

[PATCH] drm/sun4i: dsi: Prevent underflow when computing packet sizes

[Samuel Holland]

Currently, the packet overhead is subtracted using unsigned arithmetic.
With a short sync pulse, this could underflow and wrap around to near
the maximal u16 value. Fix this by using signed subtraction. The call to
max() will correctly handle any negative numbers that are produced.

Apply the same fix to the other timings, even though those subtractions
are less likely to underflow.

Fixes: 133add5b5ad4 ("drm/sun4i: Add Allwinner A31 MIPI-DSI controller support")
Signed-off-by: Samuel Holland <samuel@sholland.org>
---

 drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
index b4dfa166eccd..34234a144e87 100644
--- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
+++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
@@ -522,77 +522,77 @@ static void sun6i_dsi_setup_format(struct sun6i_dsi *dsi,
 		     SUN6I_DSI_PIXEL_PF1_CRC_INIT_LINE0(0xffff) |
 		     SUN6I_DSI_PIXEL_PF1_CRC_INIT_LINEN(0xffff));
 
 	regmap_write(dsi->regs, SUN6I_DSI_PIXEL_CTL0_REG,
 		     SUN6I_DSI_PIXEL_CTL0_PD_PLUG_DISABLE |
 		     SUN6I_DSI_PIXEL_CTL0_FORMAT(fmt));
 }
 
 static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
 				    struct drm_display_mode *mode)
 {
 	struct mipi_dsi_device *device = dsi->device;
-	unsigned int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8;
+	int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8;
 	u16 hbp = 0, hfp = 0, hsa = 0, hblk = 0, vblk = 0;
 	u32 basic_ctl = 0;
 	size_t bytes;
 	u8 *buffer;
 
 	/* Do all timing calculations up front to allocate buffer space */
 
 	if (device->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
 		hblk = mode->hdisplay * Bpp;
 		basic_ctl = SUN6I_DSI_BASIC_CTL_VIDEO_BURST |
 			    SUN6I_DSI_BASIC_CTL_HSA_HSE_DIS |
 			    SUN6I_DSI_BASIC_CTL_HBP_DIS;
 
 		if (device->lanes == 4)
 			basic_ctl |= SUN6I_DSI_BASIC_CTL_TRAIL_FILL |
 				     SUN6I_DSI_BASIC_CTL_TRAIL_INV(0xc);
 	} else {
 		/*
 		 * A sync period is composed of a blanking packet (4
 		 * bytes + payload + 2 bytes) and a sync event packet
 		 * (4 bytes). Its minimal size is therefore 10 bytes
 		 */
 #define HSA_PACKET_OVERHEAD	10
-		hsa = max((unsigned int)HSA_PACKET_OVERHEAD,
+		hsa = max(HSA_PACKET_OVERHEAD,
 			  (mode->hsync_end - mode->hsync_start) * Bpp - HSA_PACKET_OVERHEAD);
 
 		/*
 		 * The backporch is set using a blanking packet (4
 		 * bytes + payload + 2 bytes). Its minimal size is
 		 * therefore 6 bytes
 		 */
 #define HBP_PACKET_OVERHEAD	6
-		hbp = max((unsigned int)HBP_PACKET_OVERHEAD,
+		hbp = max(HBP_PACKET_OVERHEAD,
 			  (mode->htotal - mode->hsync_end) * Bpp - HBP_PACKET_OVERHEAD);
 
 		/*
 		 * The frontporch is set using a sync event (4 bytes)
 		 * and two blanking packets (each one is 4 bytes +
 		 * payload + 2 bytes). Its minimal size is therefore
 		 * 16 bytes
 		 */
 #define HFP_PACKET_OVERHEAD	16
-		hfp = max((unsigned int)HFP_PACKET_OVERHEAD,
+		hfp = max(HFP_PACKET_OVERHEAD,
 			  (mode->hsync_start - mode->hdisplay) * Bpp - HFP_PACKET_OVERHEAD);
 
 		/*
 		 * The blanking is set using a sync event (4 bytes)
 		 * and a blanking packet (4 bytes + payload + 2
 		 * bytes). Its minimal size is therefore 10 bytes.
 		 */
 #define HBLK_PACKET_OVERHEAD	10
-		hblk = max((unsigned int)HBLK_PACKET_OVERHEAD,
+		hblk = max(HBLK_PACKET_OVERHEAD,
 			   (mode->htotal - (mode->hsync_end - mode->hsync_start)) * Bpp -
 			   HBLK_PACKET_OVERHEAD);
 
 		/*
 		 * And I'm not entirely sure what vblk is about. The driver in
 		 * Allwinner BSP is using a rather convoluted calculation
 		 * there only for 4 lanes. However, using 0 (the !4 lanes
 		 * case) even with a 4 lanes screen seems to work...
 		 */
 		vblk = 0;
 	}
 
-- 
2.35.1

Re: [PATCH] drm/sun4i: dsi: Prevent underflow when computing packet sizes

[Jernej Škrabec]

Dne petek, 12. avgust 2022 ob 05:16:23 CEST je Samuel Holland napisal(a):
> Currently, the packet overhead is subtracted using unsigned arithmetic.
> With a short sync pulse, this could underflow and wrap around to near
> the maximal u16 value. Fix this by using signed subtraction. The call to
> max() will correctly handle any negative numbers that are produced.
> 
> Apply the same fix to the other timings, even though those subtractions
> are less likely to underflow.
> 
> Fixes: 133add5b5ad4 ("drm/sun4i: Add Allwinner A31 MIPI-DSI controller
> support") Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> 
>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c index b4dfa166eccd..34234a144e87
> 100644
> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
> @@ -522,77 +522,77 @@ static void sun6i_dsi_setup_format(struct sun6i_dsi
> *dsi, SUN6I_DSI_PIXEL_PF1_CRC_INIT_LINE0(0xffff) |
>  		     SUN6I_DSI_PIXEL_PF1_CRC_INIT_LINEN(0xffff));
> 
>  	regmap_write(dsi->regs, SUN6I_DSI_PIXEL_CTL0_REG,
>  		     SUN6I_DSI_PIXEL_CTL0_PD_PLUG_DISABLE |
>  		     SUN6I_DSI_PIXEL_CTL0_FORMAT(fmt));
>  }
> 
>  static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
>  				    struct drm_display_mode 
*mode)
>  {
>  	struct mipi_dsi_device *device = dsi->device;
> -	unsigned int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 
8;
> +	int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8;

Nit: mipi_dsi_pixel_format_to_bpp() can return -EINVAL in case of unsupported 
format. Would it make sense to check it?

In any case:
Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej

>  	u16 hbp = 0, hfp = 0, hsa = 0, hblk = 0, vblk = 0;
>  	u32 basic_ctl = 0;
>  	size_t bytes;
>  	u8 *buffer;
> 
>  	/* Do all timing calculations up front to allocate buffer space */
> 
>  	if (device->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
>  		hblk = mode->hdisplay * Bpp;
>  		basic_ctl = SUN6I_DSI_BASIC_CTL_VIDEO_BURST |
>  			    SUN6I_DSI_BASIC_CTL_HSA_HSE_DIS |
>  			    SUN6I_DSI_BASIC_CTL_HBP_DIS;
> 
>  		if (device->lanes == 4)
>  			basic_ctl |= SUN6I_DSI_BASIC_CTL_TRAIL_FILL 
|
>  				     
SUN6I_DSI_BASIC_CTL_TRAIL_INV(0xc);
>  	} else {
>  		/*
>  		 * A sync period is composed of a blanking packet (4
>  		 * bytes + payload + 2 bytes) and a sync event packet
>  		 * (4 bytes). Its minimal size is therefore 10 bytes
>  		 */
>  #define HSA_PACKET_OVERHEAD	10
> -		hsa = max((unsigned int)HSA_PACKET_OVERHEAD,
> +		hsa = max(HSA_PACKET_OVERHEAD,
>  			  (mode->hsync_end - mode->hsync_start) * 
Bpp - HSA_PACKET_OVERHEAD);
> 
>  		/*
>  		 * The backporch is set using a blanking packet (4
>  		 * bytes + payload + 2 bytes). Its minimal size is
>  		 * therefore 6 bytes
>  		 */
>  #define HBP_PACKET_OVERHEAD	6
> -		hbp = max((unsigned int)HBP_PACKET_OVERHEAD,
> +		hbp = max(HBP_PACKET_OVERHEAD,
>  			  (mode->htotal - mode->hsync_end) * Bpp - 
HBP_PACKET_OVERHEAD);
> 
>  		/*
>  		 * The frontporch is set using a sync event (4 bytes)
>  		 * and two blanking packets (each one is 4 bytes +
>  		 * payload + 2 bytes). Its minimal size is therefore
>  		 * 16 bytes
>  		 */
>  #define HFP_PACKET_OVERHEAD	16
> -		hfp = max((unsigned int)HFP_PACKET_OVERHEAD,
> +		hfp = max(HFP_PACKET_OVERHEAD,
>  			  (mode->hsync_start - mode->hdisplay) * Bpp 
- HFP_PACKET_OVERHEAD);
> 
>  		/*
>  		 * The blanking is set using a sync event (4 bytes)
>  		 * and a blanking packet (4 bytes + payload + 2
>  		 * bytes). Its minimal size is therefore 10 bytes.
>  		 */
>  #define HBLK_PACKET_OVERHEAD	10
> -		hblk = max((unsigned int)HBLK_PACKET_OVERHEAD,
> +		hblk = max(HBLK_PACKET_OVERHEAD,
>  			   (mode->htotal - (mode->hsync_end - mode-
>hsync_start)) * Bpp -
>  			   HBLK_PACKET_OVERHEAD);
> 
>  		/*
>  		 * And I'm not entirely sure what vblk is about. The 
driver in
>  		 * Allwinner BSP is using a rather convoluted 
calculation
>  		 * there only for 4 lanes. However, using 0 (the !4 
lanes
>  		 * case) even with a 4 lanes screen seems to work...
>  		 */
>  		vblk = 0;
>  	}

Re: [PATCH] drm/sun4i: dsi: Prevent underflow when computing packet sizes

[Samuel Holland]

On 8/14/22 2:55 AM, Jernej Škrabec wrote:
> Dne petek, 12. avgust 2022 ob 05:16:23 CEST je Samuel Holland napisal(a):
>> Currently, the packet overhead is subtracted using unsigned arithmetic.
>> With a short sync pulse, this could underflow and wrap around to near
>> the maximal u16 value. Fix this by using signed subtraction. The call to
>> max() will correctly handle any negative numbers that are produced.
>>
>> Apply the same fix to the other timings, even though those subtractions
>> are less likely to underflow.
>>
>> Fixes: 133add5b5ad4 ("drm/sun4i: Add Allwinner A31 MIPI-DSI controller
>> support") Signed-off-by: Samuel Holland <samuel@sholland.org>
>> ---
>>
>>  drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
>> b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c index b4dfa166eccd..34234a144e87
>> 100644
>> --- a/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
>> +++ b/drivers/gpu/drm/sun4i/sun6i_mipi_dsi.c
>> @@ -522,77 +522,77 @@ static void sun6i_dsi_setup_format(struct sun6i_dsi
>> *dsi, SUN6I_DSI_PIXEL_PF1_CRC_INIT_LINE0(0xffff) |
>>  		     SUN6I_DSI_PIXEL_PF1_CRC_INIT_LINEN(0xffff));
>>
>>  	regmap_write(dsi->regs, SUN6I_DSI_PIXEL_CTL0_REG,
>>  		     SUN6I_DSI_PIXEL_CTL0_PD_PLUG_DISABLE |
>>  		     SUN6I_DSI_PIXEL_CTL0_FORMAT(fmt));
>>  }
>>
>>  static void sun6i_dsi_setup_timings(struct sun6i_dsi *dsi,
>>  				    struct drm_display_mode *mode)
>>  {
>>  	struct mipi_dsi_device *device = dsi->device;
>> -	unsigned int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8;
>> +	int Bpp = mipi_dsi_pixel_format_to_bpp(device->format) / 8;
> 
> Nit: mipi_dsi_pixel_format_to_bpp() can return -EINVAL in case of unsupported 
> format. Would it make sense to check it?

The switch statement in mipi_dsi_pixel_format_to_bpp() handles every value in
the enumeration, so I think the -EINVAL is just there to keep GCC from
complaining. If we do want to handle this case, it would need to be in
sun6i_dsi_attach(), since the other places we use mipi_dsi_pixel_format_to_bpp()
are way too late to handle any errors.

Regards,
Samuel

Re: [PATCH] drm/sun4i: dsi: Prevent underflow when computing packet sizes

[Maxime Ripard]

On Thu, 11 Aug 2022 22:16:23 -0500, Samuel Holland wrote:
> Currently, the packet overhead is subtracted using unsigned arithmetic.
> With a short sync pulse, this could underflow and wrap around to near
> the maximal u16 value. Fix this by using signed subtraction. The call to
> max() will correctly handle any negative numbers that are produced.
> 
> Apply the same fix to the other timings, even though those subtractions
> are less likely to underflow.
> 
> [...]

Applied to drm/drm-misc (drm-misc-fixes).

Thanks!
Maxime