Message-ID: <4d9d5c42-842b-1152-1b81-da634fe4f124@suse.de>
Date:   Thu, 26 Nov 2020 13:51:14 +0100
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     KuoHsiang Chou <kuohsiang_chou@...eedtech.com>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Cc:     airlied@...ux.ie, tommy_huang@...eedtech.com,
        jenmin_yuan@...eedtech.com, airlied@...hat.com,
        arc_sung@...eedtech.com
Subject: Re: [PATCH] drm/ast: Fixed CVE for DP501

Hi,

please see below for a review.

Am 25.11.20 um 10:09 schrieb KuoHsiang Chou:
> [Bug][DP501]
> 1. For security concerning, P2A have to be disabled by CVE regulation.
> 2. FrameBuffer reverses last 2MB used for the image of DP501
> 3. If P2A is disallowed, the default "ioremap()" behavior is non-cached
>     and could be an alternative accessing on the image of DP501.

Please provide a more verbose description of the change. Which problem 
does this patch solve?

> ---
>   drivers/gpu/drm/ast/ast_dp501.c | 131 +++++++++++++++++++++++---------
>   drivers/gpu/drm/ast/ast_drv.h   |   2 +
>   drivers/gpu/drm/ast/ast_main.c  |  12 +++
>   drivers/gpu/drm/ast/ast_mm.c    |   1 +
>   4 files changed, 110 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c
> index 88121c0e0d05..7640364ef2bc 100644
> --- a/drivers/gpu/drm/ast/ast_dp501.c
> +++ b/drivers/gpu/drm/ast/ast_dp501.c
> @@ -189,6 +189,8 @@ bool ast_backup_fw(struct drm_device *dev, u8 *addr, u32 size)
>   	u32 i, data;
>   	u32 boot_address;
> 
> +	if (ast->config_mode != ast_use_p2a) return false;
> +
	
The coding style is incorrect. 'Return false' needs to be on the next 
line, indented by an additional tab. Here and in other place.


>   	data = ast_mindwm(ast, 0x1e6e2100) & 0x01;
>   	if (data) {
>   		boot_address = get_fw_base(ast);
> @@ -207,6 +209,8 @@ static bool ast_launch_m68k(struct drm_device *dev)
>   	u8 *fw_addr = NULL;
>   	u8 jreg;
> 
> +	if (ast->config_mode != ast_use_p2a) return false;
> +

Coding style.

>   	data = ast_mindwm(ast, 0x1e6e2100) & 0x01;
>   	if (!data) {
> 
> @@ -272,24 +276,51 @@ u8 ast_get_dp501_max_clk(struct drm_device *dev)
>   	u32 boot_address, offset, data;
>   	u8 linkcap[4], linkrate, linklanes, maxclk = 0xff;
> 
> -	boot_address = get_fw_base(ast);
> -
> -	/* validate FW version */
> -	offset = 0xf000;
> -	data = ast_mindwm(ast, boot_address + offset);
> -	if ((data & 0xf0) != 0x10) /* version: 1x */
> -		return maxclk;
> -
> -	/* Read Link Capability */
> -	offset  = 0xf014;
> -	*(u32 *)linkcap = ast_mindwm(ast, boot_address + offset);
> -	if (linkcap[2] == 0) {
> -		linkrate = linkcap[0];
> -		linklanes = linkcap[1];
> -		data = (linkrate == 0x0a) ? (90 * linklanes) : (54 * linklanes);
> -		if (data > 0xff)
> -			data = 0xff;
> -		maxclk = (u8)data;
> +	if (ast->config_mode == ast_use_p2a) {
> +		boot_address = get_fw_base(ast);
> +
> +		/* validate FW version */
> +		offset = 0xf000;
> +		data = ast_mindwm(ast, boot_address + offset);
> +		if ((data & 0xf0) != 0x10) /* version: 1x */
> +			return maxclk;

Please give these constants some meaningful names. I suggest something like

#define AST_DP501_FW_VERSION_MASK	GENMASK(7, 4)
#define AST_DP501_FW_VERSION_1		BIT(4)

There are already a few constants in ast_drv.h. I'd put them there as 
well. It's better than a comment.

> +
> +		/* Read Link Capability */
> +		offset  = 0xf014;

Please give the offset a meaningful name.


> +		*(u32 *)linkcap = ast_mindwm(ast, boot_address + offset);

The cast shoudl go to the right-hand side of the assignment.

> +		if (linkcap[2] == 0) {
> +			linkrate = linkcap[0];
> +			linklanes = linkcap[1];
> +			data = (linkrate == 0x0a) ? (90 * linklanes) : (54 * linklanes);
> +			if (data > 0xff)
> +				data = 0xff;
> +			maxclk = (u8)data;
> +		}
> +	}
> +	else {

else goes on the same line as }

> +		if (!ast->reservedbuffer) return 65;	/* 1024x768 as default */

Coding style. Please give a meaningful name to 65.

> +
> +		/* dummy read */
> +		offset = 0x0000;
> +		data = *(u32 *) (ast->reservedbuffer + offset);

Why is this required?

reservedbuffer is I/O memory accessed in 32-bit chunks. You should use 
readl and writel to access its content.

> +
> +			/* validate FW version */
> +			offset = 0xf000;

The indention is off.

> +		data = *(u32 *) (ast->reservedbuffer + offset);
> +			if ((data & 0xf0) != 0x10) /* version: 1x */
> +				return maxclk;

Indention.

> +
> +		/* Read Link Capability */
> +		offset  = 0xf014;
> +		*(u32 *)linkcap = *(u32 *) (ast->reservedbuffer + offset);
> +		if (linkcap[2] == 0) {
> +			linkrate = linkcap[0];
> +			linklanes = linkcap[1];
> +			data = (linkrate == 0x0a) ? (90 * linklanes) : (54 * linklanes);
> +			if (data > 0xff)
> +				data = 0xff;
> +			maxclk = (u8)data;
> +		}
>   	}
>   	return maxclk;
>   }
> @@ -299,25 +330,53 @@ bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata)
>   	struct ast_private *ast = to_ast_private(dev);
>   	u32 i, boot_address, offset, data;
> 
> -	boot_address = get_fw_base(ast);
> -
> -	/* validate FW version */
> -	offset = 0xf000;
> -	data = ast_mindwm(ast, boot_address + offset);
> -	if ((data & 0xf0) != 0x10)
> -		return false;
> -
> -	/* validate PnP Monitor */
> -	offset = 0xf010;
> -	data = ast_mindwm(ast, boot_address + offset);
> -	if (!(data & 0x01))
> -		return false;
> +	if (ast->config_mode == ast_use_p2a) {
> +		boot_address = get_fw_base(ast);
> 
> -	/* Read EDID */
> -	offset = 0xf020;
> -	for (i = 0; i < 128; i += 4) {
> -		data = ast_mindwm(ast, boot_address + offset + i);
> -		*(u32 *)(ediddata + i) = data;
> +		/* validate FW version */
> +		offset = 0xf000;
> +		data = ast_mindwm(ast, boot_address + offset);
> +		if ((data & 0xf0) != 0x10)
> +			return false;
> +
> +		/* validate PnP Monitor */
> +		offset = 0xf010;

Please name the constant.

> +		data = ast_mindwm(ast, boot_address + offset);
> +		if (!(data & 0x01))

Please name the constant.

> +			return false;
> +
> +		/* Read EDID */
> +		offset = 0xf020;
> +		for (i = 0; i < 128; i += 4) {
> +			data = ast_mindwm(ast, boot_address + offset + i);
> +			*(u32 *)(ediddata + i) = data;

writel for I/O access

> +		}
> +	}
> +	else {

else on wrong line

> +		if (!ast->reservedbuffer) return false;
> +
> +		/* dummy read */
> +		offset = 0x0000;
> +		data = *(u32 *) (ast->reservedbuffer + offset);
> +
> +		/* validate FW version */
> +		offset = 0xf000;
> +		data = *(u32 *) (ast->reservedbuffer + offset);
> +		if ((data & 0xf0) != 0x10)
> +			return false;
> +
> +		/* validate PnP Monitor */
> +		offset = 0xf010;
> +		data = *(u32 *) (ast->reservedbuffer + offset);
> +		if (!(data & 0x01))
> +			return false;
> +
> +		/* Read EDID */
> +		offset = 0xf020;
> +		for (i = 0; i < 128; i+=4) {
> +			data = *(u32 *) (ast->reservedbuffer + offset + i);
> +			*(u32 *)(ediddata + i) = data;
> +		}
>   	}
> 
>   	return true;
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index 6b9e3b94a712..cd17e0683fd7 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -121,12 +121,14 @@ struct ast_private {
> 
>   	void __iomem *regs;
>   	void __iomem *ioregs;
> +	void __iomem *reservedbuffer;

reservedbuffer has no meaning. As it stores the DP501's firmware, I'd 
call it dp501_fw.

> 
>   	enum ast_chip chip;
>   	bool vga2_clone;
>   	uint32_t dram_bus_width;
>   	uint32_t dram_type;
>   	uint32_t mclk;
> +	uint32_t vram_size;
> 
>   	int fb_mtrr;
> 
> diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
> index 4ec6884f6c65..4477b4cf1b06 100644
> --- a/drivers/gpu/drm/ast/ast_main.c
> +++ b/drivers/gpu/drm/ast/ast_main.c
> @@ -393,6 +393,7 @@ static void ast_device_release(void *data)
> 
>   	/* enable standard VGA decode */
>   	ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x04);
> +	pci_iounmap(ast->base.pdev, ast->reservedbuffer);
>   }
> 
>   struct ast_private *ast_device_create(struct drm_driver *drv,
> @@ -449,6 +450,17 @@ struct ast_private *ast_device_create(struct drm_driver *drv,
>   	if (ret)
>   		return ERR_PTR(ret);
> 
> +	/* map reserved buffer */
> +	ast->reservedbuffer = NULL;
> +	if (ast->vram_size < pci_resource_len(dev->pdev, 0)) {
> +		ast->reservedbuffer = ioremap( \
> +			pci_resource_start(ast->base.pdev, 0) + (unsigned long)ast->vram_size, \
> +			pci_resource_len(dev->pdev, 0) - ast->vram_size);

Use pci_iomap_range() instead. The function's offset parameter is 
vram_size, the function's maxlen parameter is 0.

You also won't need pci_iounmap(). pci_iomap_range() sets up the cleanup 
for you.

> +		if (!ast->reservedbuffer) {

No braces around single-line branch.

> +			DRM_INFO("failed to map reserved buffer! \n");

Use drm_info() instead

> +		}
> +	}
> +
>   	ret = ast_mode_config_init(ast);
>   	if (ret)
>   		return ERR_PTR(ret);
> diff --git a/drivers/gpu/drm/ast/ast_mm.c b/drivers/gpu/drm/ast/ast_mm.c
> index 8392ebde504b..c6fd24493fb3 100644
> --- a/drivers/gpu/drm/ast/ast_mm.c
> +++ b/drivers/gpu/drm/ast/ast_mm.c
> @@ -90,6 +90,7 @@ int ast_mm_init(struct ast_private *ast)
>   	int ret;
> 
>   	vram_size = ast_get_vram_size(ast);
> +	ast->vram_size = (uint32_t) vram_size;

You don't need to store vram_size. Look at dev->vram_mm->vram_size instead.

> 
>   	ret = drmm_vram_helper_init(dev, pci_resource_start(dev->pdev, 0),
>   				    vram_size);
> --
> 2.18.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

Download attachment "OpenPGP_0x680DC11D530B7A23.asc" of type "application/pgp-keys" (7898 bytes)

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)

Powered by blists - more mailing lists