RE: [PATCH 1/1] video: hyperv_fb: Fix validation of screen resolution

[Haiyang Zhang]

> -----Original Message-----
> From: Michael Kelley (LINUX) <mikelley@xxxxxxxxxxxxx>
> Sent: Sunday, January 16, 2022 2:19 PM
> To: KY Srinivasan <kys@xxxxxxxxxxxxx>; Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>; Stephen
> Hemminger <sthemmin@xxxxxxxxxxxxx>; wei.liu@xxxxxxxxxx; Wei Hu <weh@xxxxxxxxxxxxx>; Dexuan
> Cui <decui@xxxxxxxxxxxxx>; drawat.floss@xxxxxxxxx; hhei <hhei@xxxxxxxxxx>; linux-
> kernel@xxxxxxxxxxxxxxx; linux-hyperv@xxxxxxxxxxxxxxx; linux-fbdev@xxxxxxxxxxxxxxx; dri-
> devel@xxxxxxxxxxxxxxxxxxxxx
> Cc: Michael Kelley (LINUX) <mikelley@xxxxxxxxxxxxx>
> Subject: [PATCH 1/1] video: hyperv_fb: Fix validation of screen resolution
> 
> In the WIN10 version of the Synthetic Video protocol with Hyper-V,
> Hyper-V reports a list of supported resolutions as part of the protocol
> negotiation. The driver calculates the maximum width and height from
> the list of resolutions, and uses those maximums to validate any screen
> resolution specified in the video= option on the kernel boot line.
> 
> This method of validation is incorrect. For example, the list of
> supported resolutions could contain 1600x1200 and 1920x1080, both of
> which fit in an 8 Mbyte frame buffer.  But calculating the max width
> and height yields 1920 and 1200, and 1920x1200 resolution does not fit
> in an 8 Mbyte frame buffer.  Unfortunately, this resolution is accepted,
> causing a kernel fault when the driver accesses memory outside the
> frame buffer.
> 
> Instead, validate the specified screen resolution by calculating
> its size, and comparing against the frame buffer size.  Delete the
> code for calculating the max width and height from the list of
> resolutions, since these max values have no use.  Also add the
> frame buffer size to the info message to aid in understanding why
> a resolution might be rejected.
> 
> Fixes: 67e7cdb4829d ("video: hyperv: hyperv_fb: Obtain screen resolution from Hyper-V
> host")
> Signed-off-by: Michael Kelley <mikelley@xxxxxxxxxxxxx>
> ---
>  drivers/video/fbdev/hyperv_fb.c | 16 +++-------------
>  1 file changed, 3 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c
> index 23999df..c8e0ea2 100644
> --- a/drivers/video/fbdev/hyperv_fb.c
> +++ b/drivers/video/fbdev/hyperv_fb.c
> @@ -287,8 +287,6 @@ struct hvfb_par {
> 
>  static uint screen_width = HVFB_WIDTH;
>  static uint screen_height = HVFB_HEIGHT;
> -static uint screen_width_max = HVFB_WIDTH;
> -static uint screen_height_max = HVFB_HEIGHT;
>  static uint screen_depth;
>  static uint screen_fb_size;
>  static uint dio_fb_size; /* FB size for deferred IO */
> @@ -582,7 +580,6 @@ static int synthvid_get_supported_resolution(struct hv_device *hdev)
>  	int ret = 0;
>  	unsigned long t;
>  	u8 index;
> -	int i;
> 
>  	memset(msg, 0, sizeof(struct synthvid_msg));
>  	msg->vid_hdr.type = SYNTHVID_RESOLUTION_REQUEST;
> @@ -613,13 +610,6 @@ static int synthvid_get_supported_resolution(struct hv_device *hdev)
>  		goto out;
>  	}
> 
> -	for (i = 0; i < msg->resolution_resp.resolution_count; i++) {
> -		screen_width_max = max_t(unsigned int, screen_width_max,
> -		    msg->resolution_resp.supported_resolution[i].width);
> -		screen_height_max = max_t(unsigned int, screen_height_max,
> -		    msg->resolution_resp.supported_resolution[i].height);
> -	}
> -
>  	screen_width =
>  		msg->resolution_resp.supported_resolution[index].width;
>  	screen_height =
> @@ -941,7 +931,7 @@ static void hvfb_get_option(struct fb_info *info)
> 
>  	if (x < HVFB_WIDTH_MIN || y < HVFB_HEIGHT_MIN ||
>  	    (synthvid_ver_ge(par->synthvid_version, SYNTHVID_VERSION_WIN10) &&
> -	    (x > screen_width_max || y > screen_height_max)) ||
> +	    (x * y * screen_depth / 8 > screen_fb_size)) ||
>  	    (par->synthvid_version == SYNTHVID_VERSION_WIN8 &&
>  	     x * y * screen_depth / 8 > SYNTHVID_FB_SIZE_WIN8) ||
>  	    (par->synthvid_version == SYNTHVID_VERSION_WIN7 &&
> @@ -1194,8 +1184,8 @@ static int hvfb_probe(struct hv_device *hdev,
>  	}
> 
>  	hvfb_get_option(info);
> -	pr_info("Screen resolution: %dx%d, Color depth: %d\n",
> -		screen_width, screen_height, screen_depth);
> +	pr_info("Screen resolution: %dx%d, Color depth: %d, Frame buffer size: %d\n",
> +		screen_width, screen_height, screen_depth, screen_fb_size);
> 
>  	ret = hvfb_getmem(hdev, info);
>  	if (ret) {

Thank you!

Reviewed-by: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>