All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/ast: Fixed CVE for DP501
@ 2020-11-25  9:09 ` KuoHsiang Chou
  0 siblings, 0 replies; 15+ messages in thread
From: KuoHsiang Chou @ 2020-11-25  9:09 UTC (permalink / raw)
  To: tzimmermann, dri-devel, linux-kernel
  Cc: airlied, airlied, daniel, jenmin_yuan, kuohsiang_chou, arc_sung,
	tommy_huang

[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.
---
 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;
+
 	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;
+
 	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;
+
+		/* 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;
+		}
+	}
+	else {
+		if (!ast->reservedbuffer) return 65;	/* 1024x768 as default */
+
+		/* dummy read */
+		offset = 0x0000;
+		data = *(u32 *) (ast->reservedbuffer + offset);
+
+			/* validate FW version */
+			offset = 0xf000;
+		data = *(u32 *) (ast->reservedbuffer + offset);
+			if ((data & 0xf0) != 0x10) /* version: 1x */
+				return maxclk;
+
+		/* 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;
+		data = ast_mindwm(ast, boot_address + offset);
+		if (!(data & 0x01))
+			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;
+		}
+	}
+	else {
+		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;

 	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);
+		if (!ast->reservedbuffer) {
+			DRM_INFO("failed to map reserved buffer! \n");
+		}
+	}
+
 	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;

 	ret = drmm_vram_helper_init(dev, pci_resource_start(dev->pdev, 0),
 				    vram_size);
--
2.18.4


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH] drm/ast: Fixed CVE for DP501
@ 2020-11-25  9:09 ` KuoHsiang Chou
  0 siblings, 0 replies; 15+ messages in thread
From: KuoHsiang Chou @ 2020-11-25  9:09 UTC (permalink / raw)
  To: tzimmermann, dri-devel, linux-kernel
  Cc: airlied, tommy_huang, jenmin_yuan, airlied, arc_sung

[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.
---
 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;
+
 	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;
+
 	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;
+
+		/* 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;
+		}
+	}
+	else {
+		if (!ast->reservedbuffer) return 65;	/* 1024x768 as default */
+
+		/* dummy read */
+		offset = 0x0000;
+		data = *(u32 *) (ast->reservedbuffer + offset);
+
+			/* validate FW version */
+			offset = 0xf000;
+		data = *(u32 *) (ast->reservedbuffer + offset);
+			if ((data & 0xf0) != 0x10) /* version: 1x */
+				return maxclk;
+
+		/* 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;
+		data = ast_mindwm(ast, boot_address + offset);
+		if (!(data & 0x01))
+			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;
+		}
+	}
+	else {
+		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;

 	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);
+		if (!ast->reservedbuffer) {
+			DRM_INFO("failed to map reserved buffer! \n");
+		}
+	}
+
 	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;

 	ret = drmm_vram_helper_init(dev, pci_resource_start(dev->pdev, 0),
 				    vram_size);
--
2.18.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH] drm/ast: Fixed CVE for DP501
  2020-11-25  9:09 ` KuoHsiang Chou
@ 2020-11-26 12:51   ` Thomas Zimmermann
  -1 siblings, 0 replies; 15+ messages in thread
From: Thomas Zimmermann @ 2020-11-26 12:51 UTC (permalink / raw)
  To: KuoHsiang Chou, dri-devel, linux-kernel
  Cc: airlied, tommy_huang, jenmin_yuan, airlied, arc_sung


[-- Attachment #1.1.1: Type: text/plain, Size: 9640 bytes --]

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@lists.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

[-- Attachment #1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 8003 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] drm/ast: Fixed CVE for DP501
@ 2020-11-26 12:51   ` Thomas Zimmermann
  0 siblings, 0 replies; 15+ messages in thread
From: Thomas Zimmermann @ 2020-11-26 12:51 UTC (permalink / raw)
  To: KuoHsiang Chou, dri-devel, linux-kernel
  Cc: airlied, jenmin_yuan, tommy_huang, arc_sung, airlied


[-- Attachment #1.1.1.1: Type: text/plain, Size: 9640 bytes --]

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@lists.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

[-- Attachment #1.1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 8003 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] drm/ast: Fixed CVE for DP501
  2020-11-26 12:51   ` Thomas Zimmermann
@ 2020-11-26 14:26     ` Daniel Vetter
  -1 siblings, 0 replies; 15+ messages in thread
From: Daniel Vetter @ 2020-11-26 14:26 UTC (permalink / raw)
  To: Thomas Zimmermann
  Cc: KuoHsiang Chou, dri-devel, Linux Kernel Mailing List,
	Dave Airlie, jenmin_yuan, tommy_huang, arc_sung, Dave Airlie

On Thu, Nov 26, 2020 at 1:51 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> 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?

We also need a signed-off-by line per

https://dri.freedesktop.org/docs/drm/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

Otherwise we cannot merge a patch.
-Daniel

> > ---
> >   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@lists.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
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] drm/ast: Fixed CVE for DP501
@ 2020-11-26 14:26     ` Daniel Vetter
  0 siblings, 0 replies; 15+ messages in thread
From: Daniel Vetter @ 2020-11-26 14:26 UTC (permalink / raw)
  To: Thomas Zimmermann
  Cc: Dave Airlie, Linux Kernel Mailing List, dri-devel, tommy_huang,
	jenmin_yuan, Dave Airlie, arc_sung

On Thu, Nov 26, 2020 at 1:51 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> 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?

We also need a signed-off-by line per

https://dri.freedesktop.org/docs/drm/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin

Otherwise we cannot merge a patch.
-Daniel

> > ---
> >   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@lists.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
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2] drm/ast: Fixed CVE for DP501
  2020-11-26 12:51   ` Thomas Zimmermann
@ 2020-12-11  8:22     ` KuoHsiang Chou
  -1 siblings, 0 replies; 15+ messages in thread
From: KuoHsiang Chou @ 2020-12-11  8:22 UTC (permalink / raw)
  To: tzimmermann, dri-devel, linux-kernel
  Cc: airlied, airlied, daniel, jenmin_yuan, kuohsiang_chou, arc_sung,
	tommy_huang

[Bug][DP501]
If ASPEED P2A (PCI to AHB) bridge is disabled and disallowed for
CVE_2019_6260 item3, and then the monitor's EDID is unable read through
Parade DP501.
The reason is the DP501's FW is mapped to BMC addressing space rather
than Host addressing space.
The resolution is that using "pci_iomap_range()" maps to DP501's FW that
stored on the end of FB (Frame Buffer).
In this case, FrameBuffer reserves the last 2MB used for the image of
DP501.

Signed-off-by: KuoHsiang Chou <kuohsiang_chou@aspeedtech.com>
---
 drivers/gpu/drm/ast/ast_dp501.c | 136 +++++++++++++++++++++++---------
 drivers/gpu/drm/ast/ast_drv.h   |  12 +++
 drivers/gpu/drm/ast/ast_main.c  |   8 ++
 3 files changed, 120 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c
index 88121c0e0d05..aef9bbace99f 100644
--- a/drivers/gpu/drm/ast/ast_dp501.c
+++ b/drivers/gpu/drm/ast/ast_dp501.c
@@ -189,6 +189,9 @@ 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;
+
 	data = ast_mindwm(ast, 0x1e6e2100) & 0x01;
 	if (data) {
 		boot_address = get_fw_base(ast);
@@ -207,6 +210,9 @@ static bool ast_launch_m68k(struct drm_device *dev)
 	u8 *fw_addr = NULL;
 	u8 jreg;

+	if (ast->config_mode != ast_use_p2a)
+		return false;
+
 	data = ast_mindwm(ast, 0x1e6e2100) & 0x01;
 	if (!data) {

@@ -271,25 +277,55 @@ u8 ast_get_dp501_max_clk(struct drm_device *dev)
 	struct ast_private *ast = to_ast_private(dev);
 	u32 boot_address, offset, data;
 	u8 linkcap[4], linkrate, linklanes, maxclk = 0xff;
+	u32 *plinkcap;

-	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 = AST_DP501_GBL_VERSION;
+		data = ast_mindwm(ast, boot_address + offset);
+		if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1) /* version: 1x */
+			return maxclk;
+
+		/* Read Link Capability */
+		offset  = AST_DP501_LINKRATE;
+		plinkcap = (u32 *)linkcap;
+		*plinkcap  = 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;
+		}
+	} else {
+		if (!ast->dp501_fw_buf)
+			return AST_DP501_DEFAULT_DCLK;	/* 1024x768 as default */
+
+		/* dummy read */
+		offset = 0x0000;
+		data = readl(ast->dp501_fw_buf + offset);
+
+		/* validate FW version */
+		offset = AST_DP501_GBL_VERSION;
+		data = readl(ast->dp501_fw_buf + offset);
+		if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1) /* version: 1x */
+			return maxclk;
+
+		/* Read Link Capability */
+		offset = AST_DP501_LINKRATE;
+		plinkcap = (u32 *)linkcap;
+		*plinkcap = readl(ast->dp501_fw_buf + 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 +335,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 = AST_DP501_GBL_VERSION;
+		data = ast_mindwm(ast, boot_address + offset);
+		if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1)
+			return false;
+
+		/* validate PnP Monitor */
+		offset = AST_DP501_PNPMONITOR;
+		data = ast_mindwm(ast, boot_address + offset);
+		if (!(data & AST_DP501_PNP_CONNECTED))
+			return false;
+
+		/* Read EDID */
+		offset = AST_DP501_EDID_DATA;
+		for (i = 0; i < 128; i += 4) {
+			data = ast_mindwm(ast, boot_address + offset + i);
+			writel(data, (u32 *)(ediddata + i));
+		}
+	} else {
+		if (!ast->dp501_fw_buf)
+			return false;
+
+		/* dummy read */
+		offset = 0x0000;
+		data = readl(ast->dp501_fw_buf + offset);
+
+		/* validate FW version */
+		offset = AST_DP501_GBL_VERSION;
+		data = readl(ast->dp501_fw_buf + offset);
+		if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1)
+			return false;
+
+		/* validate PnP Monitor */
+		offset = AST_DP501_PNPMONITOR;
+		data = readl(ast->dp501_fw_buf + offset);
+		if (!(data & AST_DP501_PNP_CONNECTED))
+			return false;
+
+		/* Read EDID */
+		offset = AST_DP501_EDID_DATA;
+		for (i = 0; i < 128; i += 4) {
+			data = readl(ast->dp501_fw_buf + offset + i);
+			writel(data, (u32 *)(ediddata + i));
+		}
 	}

 	return true;
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 6b9e3b94a712..da6dfb677540 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -121,6 +121,7 @@ struct ast_private {

 	void __iomem *regs;
 	void __iomem *ioregs;
+	void __iomem *dp501_fw_buf;

 	enum ast_chip chip;
 	bool vga2_clone;
@@ -299,6 +300,17 @@ int ast_mode_config_init(struct ast_private *ast);
 #define AST_MM_ALIGN_SHIFT 4
 #define AST_MM_ALIGN_MASK ((1 << AST_MM_ALIGN_SHIFT) - 1)

+#define AST_DP501_FW_VERSION_MASK	GENMASK(7, 4)
+#define AST_DP501_FW_VERSION_1		BIT(4)
+#define AST_DP501_PNP_CONNECTED	BIT(1)
+
+#define AST_DP501_DEFAULT_DCLK	65
+
+#define AST_DP501_GBL_VERSION	0xf000
+#define AST_DP501_PNPMONITOR	0xf010
+#define AST_DP501_LINKRATE	0xf014
+#define AST_DP501_EDID_DATA	0xf020
+
 int ast_mm_init(struct ast_private *ast);

 /* ast post */
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 4ec6884f6c65..3775fe26f792 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -449,6 +449,14 @@ struct ast_private *ast_device_create(struct drm_driver *drv,
 	if (ret)
 		return ERR_PTR(ret);

+	/* map reserved buffer */
+	ast->dp501_fw_buf = NULL;
+	if (dev->vram_mm->vram_size < pci_resource_len(dev->pdev, 0)) {
+		ast->dp501_fw_buf = pci_iomap_range(dev->pdev, 0, dev->vram_mm->vram_size, 0);
+		if (!ast->dp501_fw_buf)
+			drm_info(dev, "failed to map reserved buffer!\n");
+	}
+
 	ret = ast_mode_config_init(ast);
 	if (ret)
 		return ERR_PTR(ret);
--
2.18.4


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH v2] drm/ast: Fixed CVE for DP501
@ 2020-12-11  8:22     ` KuoHsiang Chou
  0 siblings, 0 replies; 15+ messages in thread
From: KuoHsiang Chou @ 2020-12-11  8:22 UTC (permalink / raw)
  To: tzimmermann, dri-devel, linux-kernel
  Cc: airlied, tommy_huang, jenmin_yuan, airlied, arc_sung

[Bug][DP501]
If ASPEED P2A (PCI to AHB) bridge is disabled and disallowed for
CVE_2019_6260 item3, and then the monitor's EDID is unable read through
Parade DP501.
The reason is the DP501's FW is mapped to BMC addressing space rather
than Host addressing space.
The resolution is that using "pci_iomap_range()" maps to DP501's FW that
stored on the end of FB (Frame Buffer).
In this case, FrameBuffer reserves the last 2MB used for the image of
DP501.

Signed-off-by: KuoHsiang Chou <kuohsiang_chou@aspeedtech.com>
---
 drivers/gpu/drm/ast/ast_dp501.c | 136 +++++++++++++++++++++++---------
 drivers/gpu/drm/ast/ast_drv.h   |  12 +++
 drivers/gpu/drm/ast/ast_main.c  |   8 ++
 3 files changed, 120 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c
index 88121c0e0d05..aef9bbace99f 100644
--- a/drivers/gpu/drm/ast/ast_dp501.c
+++ b/drivers/gpu/drm/ast/ast_dp501.c
@@ -189,6 +189,9 @@ 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;
+
 	data = ast_mindwm(ast, 0x1e6e2100) & 0x01;
 	if (data) {
 		boot_address = get_fw_base(ast);
@@ -207,6 +210,9 @@ static bool ast_launch_m68k(struct drm_device *dev)
 	u8 *fw_addr = NULL;
 	u8 jreg;

+	if (ast->config_mode != ast_use_p2a)
+		return false;
+
 	data = ast_mindwm(ast, 0x1e6e2100) & 0x01;
 	if (!data) {

@@ -271,25 +277,55 @@ u8 ast_get_dp501_max_clk(struct drm_device *dev)
 	struct ast_private *ast = to_ast_private(dev);
 	u32 boot_address, offset, data;
 	u8 linkcap[4], linkrate, linklanes, maxclk = 0xff;
+	u32 *plinkcap;

-	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 = AST_DP501_GBL_VERSION;
+		data = ast_mindwm(ast, boot_address + offset);
+		if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1) /* version: 1x */
+			return maxclk;
+
+		/* Read Link Capability */
+		offset  = AST_DP501_LINKRATE;
+		plinkcap = (u32 *)linkcap;
+		*plinkcap  = 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;
+		}
+	} else {
+		if (!ast->dp501_fw_buf)
+			return AST_DP501_DEFAULT_DCLK;	/* 1024x768 as default */
+
+		/* dummy read */
+		offset = 0x0000;
+		data = readl(ast->dp501_fw_buf + offset);
+
+		/* validate FW version */
+		offset = AST_DP501_GBL_VERSION;
+		data = readl(ast->dp501_fw_buf + offset);
+		if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1) /* version: 1x */
+			return maxclk;
+
+		/* Read Link Capability */
+		offset = AST_DP501_LINKRATE;
+		plinkcap = (u32 *)linkcap;
+		*plinkcap = readl(ast->dp501_fw_buf + 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 +335,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 = AST_DP501_GBL_VERSION;
+		data = ast_mindwm(ast, boot_address + offset);
+		if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1)
+			return false;
+
+		/* validate PnP Monitor */
+		offset = AST_DP501_PNPMONITOR;
+		data = ast_mindwm(ast, boot_address + offset);
+		if (!(data & AST_DP501_PNP_CONNECTED))
+			return false;
+
+		/* Read EDID */
+		offset = AST_DP501_EDID_DATA;
+		for (i = 0; i < 128; i += 4) {
+			data = ast_mindwm(ast, boot_address + offset + i);
+			writel(data, (u32 *)(ediddata + i));
+		}
+	} else {
+		if (!ast->dp501_fw_buf)
+			return false;
+
+		/* dummy read */
+		offset = 0x0000;
+		data = readl(ast->dp501_fw_buf + offset);
+
+		/* validate FW version */
+		offset = AST_DP501_GBL_VERSION;
+		data = readl(ast->dp501_fw_buf + offset);
+		if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1)
+			return false;
+
+		/* validate PnP Monitor */
+		offset = AST_DP501_PNPMONITOR;
+		data = readl(ast->dp501_fw_buf + offset);
+		if (!(data & AST_DP501_PNP_CONNECTED))
+			return false;
+
+		/* Read EDID */
+		offset = AST_DP501_EDID_DATA;
+		for (i = 0; i < 128; i += 4) {
+			data = readl(ast->dp501_fw_buf + offset + i);
+			writel(data, (u32 *)(ediddata + i));
+		}
 	}

 	return true;
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 6b9e3b94a712..da6dfb677540 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -121,6 +121,7 @@ struct ast_private {

 	void __iomem *regs;
 	void __iomem *ioregs;
+	void __iomem *dp501_fw_buf;

 	enum ast_chip chip;
 	bool vga2_clone;
@@ -299,6 +300,17 @@ int ast_mode_config_init(struct ast_private *ast);
 #define AST_MM_ALIGN_SHIFT 4
 #define AST_MM_ALIGN_MASK ((1 << AST_MM_ALIGN_SHIFT) - 1)

+#define AST_DP501_FW_VERSION_MASK	GENMASK(7, 4)
+#define AST_DP501_FW_VERSION_1		BIT(4)
+#define AST_DP501_PNP_CONNECTED	BIT(1)
+
+#define AST_DP501_DEFAULT_DCLK	65
+
+#define AST_DP501_GBL_VERSION	0xf000
+#define AST_DP501_PNPMONITOR	0xf010
+#define AST_DP501_LINKRATE	0xf014
+#define AST_DP501_EDID_DATA	0xf020
+
 int ast_mm_init(struct ast_private *ast);

 /* ast post */
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 4ec6884f6c65..3775fe26f792 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -449,6 +449,14 @@ struct ast_private *ast_device_create(struct drm_driver *drv,
 	if (ret)
 		return ERR_PTR(ret);

+	/* map reserved buffer */
+	ast->dp501_fw_buf = NULL;
+	if (dev->vram_mm->vram_size < pci_resource_len(dev->pdev, 0)) {
+		ast->dp501_fw_buf = pci_iomap_range(dev->pdev, 0, dev->vram_mm->vram_size, 0);
+		if (!ast->dp501_fw_buf)
+			drm_info(dev, "failed to map reserved buffer!\n");
+	}
+
 	ret = ast_mode_config_init(ast);
 	if (ret)
 		return ERR_PTR(ret);
--
2.18.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] drm/ast: Fixed CVE for DP501
  2020-12-11  8:22     ` KuoHsiang Chou
  (?)
@ 2020-12-11 18:52       ` kernel test robot
  -1 siblings, 0 replies; 15+ messages in thread
From: kernel test robot @ 2020-12-11 18:52 UTC (permalink / raw)
  To: KuoHsiang Chou, tzimmermann, dri-devel, linux-kernel
  Cc: kbuild-all, airlied, tommy_huang, jenmin_yuan, airlied, arc_sung

[-- Attachment #1: Type: text/plain, Size: 4317 bytes --]

Hi KuoHsiang,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm-exynos/exynos-drm-next]
[also build test WARNING on drm-intel/for-linux-next tegra-drm/drm/tegra/for-next drm-tip/drm-tip linus/master v5.10-rc7 next-20201211]
[cannot apply to drm/drm-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/KuoHsiang-Chou/drm-ast-Fixed-CVE-for-DP501/20201211-162352
base:   https://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git exynos-drm-next
config: x86_64-randconfig-s022-20201210 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-179-ga00755aa-dirty
        # https://github.com/0day-ci/linux/commit/75af180bfa7bc2227224653381d743b9396b41c2
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review KuoHsiang-Chou/drm-ast-Fixed-CVE-for-DP501/20201211-162352
        git checkout 75af180bfa7bc2227224653381d743b9396b41c2
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


"sparse warnings: (new ones prefixed by >>)"
>> drivers/gpu/drm/ast/ast_dp501.c:357:39: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected void volatile [noderef] __iomem *addr @@     got unsigned int [usertype] * @@
   drivers/gpu/drm/ast/ast_dp501.c:357:39: sparse:     expected void volatile [noderef] __iomem *addr
   drivers/gpu/drm/ast/ast_dp501.c:357:39: sparse:     got unsigned int [usertype] *
   drivers/gpu/drm/ast/ast_dp501.c:383:39: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected void volatile [noderef] __iomem *addr @@     got unsigned int [usertype] * @@
   drivers/gpu/drm/ast/ast_dp501.c:383:39: sparse:     expected void volatile [noderef] __iomem *addr
   drivers/gpu/drm/ast/ast_dp501.c:383:39: sparse:     got unsigned int [usertype] *

vim +357 drivers/gpu/drm/ast/ast_dp501.c

   332	
   333	bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata)
   334	{
   335		struct ast_private *ast = to_ast_private(dev);
   336		u32 i, boot_address, offset, data;
   337	
   338		if (ast->config_mode == ast_use_p2a) {
   339			boot_address = get_fw_base(ast);
   340	
   341			/* validate FW version */
   342			offset = AST_DP501_GBL_VERSION;
   343			data = ast_mindwm(ast, boot_address + offset);
   344			if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1)
   345				return false;
   346	
   347			/* validate PnP Monitor */
   348			offset = AST_DP501_PNPMONITOR;
   349			data = ast_mindwm(ast, boot_address + offset);
   350			if (!(data & AST_DP501_PNP_CONNECTED))
   351				return false;
   352	
   353			/* Read EDID */
   354			offset = AST_DP501_EDID_DATA;
   355			for (i = 0; i < 128; i += 4) {
   356				data = ast_mindwm(ast, boot_address + offset + i);
 > 357				writel(data, (u32 *)(ediddata + i));
   358			}
   359		} else {
   360			if (!ast->dp501_fw_buf)
   361				return false;
   362	
   363			/* dummy read */
   364			offset = 0x0000;
   365			data = readl(ast->dp501_fw_buf + offset);
   366	
   367			/* validate FW version */
   368			offset = AST_DP501_GBL_VERSION;
   369			data = readl(ast->dp501_fw_buf + offset);
   370			if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1)
   371				return false;
   372	
   373			/* validate PnP Monitor */
   374			offset = AST_DP501_PNPMONITOR;
   375			data = readl(ast->dp501_fw_buf + offset);
   376			if (!(data & AST_DP501_PNP_CONNECTED))
   377				return false;
   378	
   379			/* Read EDID */
   380			offset = AST_DP501_EDID_DATA;
   381			for (i = 0; i < 128; i += 4) {
   382				data = readl(ast->dp501_fw_buf + offset + i);
   383				writel(data, (u32 *)(ediddata + i));
   384			}
   385		}
   386	
   387		return true;
   388	}
   389	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 38525 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] drm/ast: Fixed CVE for DP501
@ 2020-12-11 18:52       ` kernel test robot
  0 siblings, 0 replies; 15+ messages in thread
From: kernel test robot @ 2020-12-11 18:52 UTC (permalink / raw)
  To: KuoHsiang Chou, tzimmermann, dri-devel, linux-kernel
  Cc: kbuild-all, airlied, tommy_huang, jenmin_yuan, airlied, arc_sung

[-- Attachment #1: Type: text/plain, Size: 4317 bytes --]

Hi KuoHsiang,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm-exynos/exynos-drm-next]
[also build test WARNING on drm-intel/for-linux-next tegra-drm/drm/tegra/for-next drm-tip/drm-tip linus/master v5.10-rc7 next-20201211]
[cannot apply to drm/drm-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/KuoHsiang-Chou/drm-ast-Fixed-CVE-for-DP501/20201211-162352
base:   https://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git exynos-drm-next
config: x86_64-randconfig-s022-20201210 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-179-ga00755aa-dirty
        # https://github.com/0day-ci/linux/commit/75af180bfa7bc2227224653381d743b9396b41c2
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review KuoHsiang-Chou/drm-ast-Fixed-CVE-for-DP501/20201211-162352
        git checkout 75af180bfa7bc2227224653381d743b9396b41c2
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


"sparse warnings: (new ones prefixed by >>)"
>> drivers/gpu/drm/ast/ast_dp501.c:357:39: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected void volatile [noderef] __iomem *addr @@     got unsigned int [usertype] * @@
   drivers/gpu/drm/ast/ast_dp501.c:357:39: sparse:     expected void volatile [noderef] __iomem *addr
   drivers/gpu/drm/ast/ast_dp501.c:357:39: sparse:     got unsigned int [usertype] *
   drivers/gpu/drm/ast/ast_dp501.c:383:39: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected void volatile [noderef] __iomem *addr @@     got unsigned int [usertype] * @@
   drivers/gpu/drm/ast/ast_dp501.c:383:39: sparse:     expected void volatile [noderef] __iomem *addr
   drivers/gpu/drm/ast/ast_dp501.c:383:39: sparse:     got unsigned int [usertype] *

vim +357 drivers/gpu/drm/ast/ast_dp501.c

   332	
   333	bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata)
   334	{
   335		struct ast_private *ast = to_ast_private(dev);
   336		u32 i, boot_address, offset, data;
   337	
   338		if (ast->config_mode == ast_use_p2a) {
   339			boot_address = get_fw_base(ast);
   340	
   341			/* validate FW version */
   342			offset = AST_DP501_GBL_VERSION;
   343			data = ast_mindwm(ast, boot_address + offset);
   344			if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1)
   345				return false;
   346	
   347			/* validate PnP Monitor */
   348			offset = AST_DP501_PNPMONITOR;
   349			data = ast_mindwm(ast, boot_address + offset);
   350			if (!(data & AST_DP501_PNP_CONNECTED))
   351				return false;
   352	
   353			/* Read EDID */
   354			offset = AST_DP501_EDID_DATA;
   355			for (i = 0; i < 128; i += 4) {
   356				data = ast_mindwm(ast, boot_address + offset + i);
 > 357				writel(data, (u32 *)(ediddata + i));
   358			}
   359		} else {
   360			if (!ast->dp501_fw_buf)
   361				return false;
   362	
   363			/* dummy read */
   364			offset = 0x0000;
   365			data = readl(ast->dp501_fw_buf + offset);
   366	
   367			/* validate FW version */
   368			offset = AST_DP501_GBL_VERSION;
   369			data = readl(ast->dp501_fw_buf + offset);
   370			if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1)
   371				return false;
   372	
   373			/* validate PnP Monitor */
   374			offset = AST_DP501_PNPMONITOR;
   375			data = readl(ast->dp501_fw_buf + offset);
   376			if (!(data & AST_DP501_PNP_CONNECTED))
   377				return false;
   378	
   379			/* Read EDID */
   380			offset = AST_DP501_EDID_DATA;
   381			for (i = 0; i < 128; i += 4) {
   382				data = readl(ast->dp501_fw_buf + offset + i);
   383				writel(data, (u32 *)(ediddata + i));
   384			}
   385		}
   386	
   387		return true;
   388	}
   389	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 38525 bytes --]

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] drm/ast: Fixed CVE for DP501
@ 2020-12-11 18:52       ` kernel test robot
  0 siblings, 0 replies; 15+ messages in thread
From: kernel test robot @ 2020-12-11 18:52 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 4420 bytes --]

Hi KuoHsiang,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on drm-exynos/exynos-drm-next]
[also build test WARNING on drm-intel/for-linux-next tegra-drm/drm/tegra/for-next drm-tip/drm-tip linus/master v5.10-rc7 next-20201211]
[cannot apply to drm/drm-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/KuoHsiang-Chou/drm-ast-Fixed-CVE-for-DP501/20201211-162352
base:   https://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git exynos-drm-next
config: x86_64-randconfig-s022-20201210 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-179-ga00755aa-dirty
        # https://github.com/0day-ci/linux/commit/75af180bfa7bc2227224653381d743b9396b41c2
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review KuoHsiang-Chou/drm-ast-Fixed-CVE-for-DP501/20201211-162352
        git checkout 75af180bfa7bc2227224653381d743b9396b41c2
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


"sparse warnings: (new ones prefixed by >>)"
>> drivers/gpu/drm/ast/ast_dp501.c:357:39: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected void volatile [noderef] __iomem *addr @@     got unsigned int [usertype] * @@
   drivers/gpu/drm/ast/ast_dp501.c:357:39: sparse:     expected void volatile [noderef] __iomem *addr
   drivers/gpu/drm/ast/ast_dp501.c:357:39: sparse:     got unsigned int [usertype] *
   drivers/gpu/drm/ast/ast_dp501.c:383:39: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected void volatile [noderef] __iomem *addr @@     got unsigned int [usertype] * @@
   drivers/gpu/drm/ast/ast_dp501.c:383:39: sparse:     expected void volatile [noderef] __iomem *addr
   drivers/gpu/drm/ast/ast_dp501.c:383:39: sparse:     got unsigned int [usertype] *

vim +357 drivers/gpu/drm/ast/ast_dp501.c

   332	
   333	bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata)
   334	{
   335		struct ast_private *ast = to_ast_private(dev);
   336		u32 i, boot_address, offset, data;
   337	
   338		if (ast->config_mode == ast_use_p2a) {
   339			boot_address = get_fw_base(ast);
   340	
   341			/* validate FW version */
   342			offset = AST_DP501_GBL_VERSION;
   343			data = ast_mindwm(ast, boot_address + offset);
   344			if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1)
   345				return false;
   346	
   347			/* validate PnP Monitor */
   348			offset = AST_DP501_PNPMONITOR;
   349			data = ast_mindwm(ast, boot_address + offset);
   350			if (!(data & AST_DP501_PNP_CONNECTED))
   351				return false;
   352	
   353			/* Read EDID */
   354			offset = AST_DP501_EDID_DATA;
   355			for (i = 0; i < 128; i += 4) {
   356				data = ast_mindwm(ast, boot_address + offset + i);
 > 357				writel(data, (u32 *)(ediddata + i));
   358			}
   359		} else {
   360			if (!ast->dp501_fw_buf)
   361				return false;
   362	
   363			/* dummy read */
   364			offset = 0x0000;
   365			data = readl(ast->dp501_fw_buf + offset);
   366	
   367			/* validate FW version */
   368			offset = AST_DP501_GBL_VERSION;
   369			data = readl(ast->dp501_fw_buf + offset);
   370			if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1)
   371				return false;
   372	
   373			/* validate PnP Monitor */
   374			offset = AST_DP501_PNPMONITOR;
   375			data = readl(ast->dp501_fw_buf + offset);
   376			if (!(data & AST_DP501_PNP_CONNECTED))
   377				return false;
   378	
   379			/* Read EDID */
   380			offset = AST_DP501_EDID_DATA;
   381			for (i = 0; i < 128; i += 4) {
   382				data = readl(ast->dp501_fw_buf + offset + i);
   383				writel(data, (u32 *)(ediddata + i));
   384			}
   385		}
   386	
   387		return true;
   388	}
   389	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 38525 bytes --]

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH V3] drm/ast: Fixed CVE for DP501
  2020-11-26 12:51   ` Thomas Zimmermann
@ 2020-12-28  3:06     ` KuoHsiang Chou
  -1 siblings, 0 replies; 15+ messages in thread
From: KuoHsiang Chou @ 2020-12-28  3:06 UTC (permalink / raw)
  To: tzimmermann, dri-devel, linux-kernel
  Cc: airlied, airlied, daniel, jenmin_yuan, kuohsiang_chou, arc_sung,
	tommy_huang

[Bug][DP501]
If ASPEED P2A (PCI to AHB) bridge is disabled and disallowed for
CVE_2019_6260 item3, and then the monitor's EDID is unable read through
Parade DP501.
The reason is the DP501's FW is mapped to BMC addressing space rather
than Host addressing space.
The resolution is that using "pci_iomap_range()" maps to DP501's FW that
stored on the end of FB (Frame Buffer).
0In this case, FrameBuffer reserves the last 2MB used for the image of
DP501.

Signed-off-by: KuoHsiang Chou <kuohsiang_chou@aspeedtech.com>
Reported-by: kernel test robot <lkp@intel.com>
---
 drivers/gpu/drm/ast/ast_dp501.c | 139 +++++++++++++++++++++++---------
 drivers/gpu/drm/ast/ast_drv.h   |  12 +++
 drivers/gpu/drm/ast/ast_main.c  |   8 ++
 3 files changed, 123 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c
index 88121c0e0d05..cd93c44f2662 100644
--- a/drivers/gpu/drm/ast/ast_dp501.c
+++ b/drivers/gpu/drm/ast/ast_dp501.c
@@ -189,6 +189,9 @@ 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;
+
 	data = ast_mindwm(ast, 0x1e6e2100) & 0x01;
 	if (data) {
 		boot_address = get_fw_base(ast);
@@ -207,6 +210,9 @@ static bool ast_launch_m68k(struct drm_device *dev)
 	u8 *fw_addr = NULL;
 	u8 jreg;

+	if (ast->config_mode != ast_use_p2a)
+		return false;
+
 	data = ast_mindwm(ast, 0x1e6e2100) & 0x01;
 	if (!data) {

@@ -271,25 +277,55 @@ u8 ast_get_dp501_max_clk(struct drm_device *dev)
 	struct ast_private *ast = to_ast_private(dev);
 	u32 boot_address, offset, data;
 	u8 linkcap[4], linkrate, linklanes, maxclk = 0xff;
+	u32 *plinkcap;

-	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 = AST_DP501_GBL_VERSION;
+		data = ast_mindwm(ast, boot_address + offset);
+		if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1) /* version: 1x */
+			return maxclk;
+
+		/* Read Link Capability */
+		offset  = AST_DP501_LINKRATE;
+		plinkcap = (u32 *)linkcap;
+		*plinkcap  = 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;
+		}
+	} else {
+		if (!ast->dp501_fw_buf)
+			return AST_DP501_DEFAULT_DCLK;	/* 1024x768 as default */
+
+		/* dummy read */
+		offset = 0x0000;
+		data = readl(ast->dp501_fw_buf + offset);
+
+		/* validate FW version */
+		offset = AST_DP501_GBL_VERSION;
+		data = readl(ast->dp501_fw_buf + offset);
+		if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1) /* version: 1x */
+			return maxclk;
+
+		/* Read Link Capability */
+		offset = AST_DP501_LINKRATE;
+		plinkcap = (u32 *)linkcap;
+		*plinkcap = readl(ast->dp501_fw_buf + 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;
 }
@@ -298,26 +334,57 @@ 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;
+	u32 *pEDIDidx;

-	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 = AST_DP501_GBL_VERSION;
+		data = ast_mindwm(ast, boot_address + offset);
+		if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1)
+			return false;
+
+		/* validate PnP Monitor */
+		offset = AST_DP501_PNPMONITOR;
+		data = ast_mindwm(ast, boot_address + offset);
+		if (!(data & AST_DP501_PNP_CONNECTED))
+			return false;
+
+		/* Read EDID */
+		offset = AST_DP501_EDID_DATA;
+		for (i = 0; i < 128; i += 4) {
+			data = ast_mindwm(ast, boot_address + offset + i);
+			pEDIDidx = (u32 *)(ediddata + i);
+			*pEDIDidx = data;
+		}
+	} else {
+		if (!ast->dp501_fw_buf)
+			return false;
+
+		/* dummy read */
+		offset = 0x0000;
+		data = readl(ast->dp501_fw_buf + offset);
+
+		/* validate FW version */
+		offset = AST_DP501_GBL_VERSION;
+		data = readl(ast->dp501_fw_buf + offset);
+		if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1)
+			return false;
+
+		/* validate PnP Monitor */
+		offset = AST_DP501_PNPMONITOR;
+		data = readl(ast->dp501_fw_buf + offset);
+		if (!(data & AST_DP501_PNP_CONNECTED))
+			return false;
+
+		/* Read EDID */
+		offset = AST_DP501_EDID_DATA;
+		for (i = 0; i < 128; i += 4) {
+			data = readl(ast->dp501_fw_buf + offset + i);
+			pEDIDidx = (u32 *)(ediddata + i);
+			*pEDIDidx = data;
+		}
 	}

 	return true;
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 6b9e3b94a712..da6dfb677540 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -121,6 +121,7 @@ struct ast_private {

 	void __iomem *regs;
 	void __iomem *ioregs;
+	void __iomem *dp501_fw_buf;

 	enum ast_chip chip;
 	bool vga2_clone;
@@ -299,6 +300,17 @@ int ast_mode_config_init(struct ast_private *ast);
 #define AST_MM_ALIGN_SHIFT 4
 #define AST_MM_ALIGN_MASK ((1 << AST_MM_ALIGN_SHIFT) - 1)

+#define AST_DP501_FW_VERSION_MASK	GENMASK(7, 4)
+#define AST_DP501_FW_VERSION_1		BIT(4)
+#define AST_DP501_PNP_CONNECTED		BIT(1)
+
+#define AST_DP501_DEFAULT_DCLK	65
+
+#define AST_DP501_GBL_VERSION	0xf000
+#define AST_DP501_PNPMONITOR	0xf010
+#define AST_DP501_LINKRATE	0xf014
+#define AST_DP501_EDID_DATA	0xf020
+
 int ast_mm_init(struct ast_private *ast);

 /* ast post */
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 4ec6884f6c65..3775fe26f792 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -449,6 +449,14 @@ struct ast_private *ast_device_create(struct drm_driver *drv,
 	if (ret)
 		return ERR_PTR(ret);

+	/* map reserved buffer */
+	ast->dp501_fw_buf = NULL;
+	if (dev->vram_mm->vram_size < pci_resource_len(dev->pdev, 0)) {
+		ast->dp501_fw_buf = pci_iomap_range(dev->pdev, 0, dev->vram_mm->vram_size, 0);
+		if (!ast->dp501_fw_buf)
+			drm_info(dev, "failed to map reserved buffer!\n");
+	}
+
 	ret = ast_mode_config_init(ast);
 	if (ret)
 		return ERR_PTR(ret);
--
2.18.4


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH V3] drm/ast: Fixed CVE for DP501
@ 2020-12-28  3:06     ` KuoHsiang Chou
  0 siblings, 0 replies; 15+ messages in thread
From: KuoHsiang Chou @ 2020-12-28  3:06 UTC (permalink / raw)
  To: tzimmermann, dri-devel, linux-kernel
  Cc: airlied, tommy_huang, jenmin_yuan, airlied, arc_sung

[Bug][DP501]
If ASPEED P2A (PCI to AHB) bridge is disabled and disallowed for
CVE_2019_6260 item3, and then the monitor's EDID is unable read through
Parade DP501.
The reason is the DP501's FW is mapped to BMC addressing space rather
than Host addressing space.
The resolution is that using "pci_iomap_range()" maps to DP501's FW that
stored on the end of FB (Frame Buffer).
0In this case, FrameBuffer reserves the last 2MB used for the image of
DP501.

Signed-off-by: KuoHsiang Chou <kuohsiang_chou@aspeedtech.com>
Reported-by: kernel test robot <lkp@intel.com>
---
 drivers/gpu/drm/ast/ast_dp501.c | 139 +++++++++++++++++++++++---------
 drivers/gpu/drm/ast/ast_drv.h   |  12 +++
 drivers/gpu/drm/ast/ast_main.c  |   8 ++
 3 files changed, 123 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c
index 88121c0e0d05..cd93c44f2662 100644
--- a/drivers/gpu/drm/ast/ast_dp501.c
+++ b/drivers/gpu/drm/ast/ast_dp501.c
@@ -189,6 +189,9 @@ 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;
+
 	data = ast_mindwm(ast, 0x1e6e2100) & 0x01;
 	if (data) {
 		boot_address = get_fw_base(ast);
@@ -207,6 +210,9 @@ static bool ast_launch_m68k(struct drm_device *dev)
 	u8 *fw_addr = NULL;
 	u8 jreg;

+	if (ast->config_mode != ast_use_p2a)
+		return false;
+
 	data = ast_mindwm(ast, 0x1e6e2100) & 0x01;
 	if (!data) {

@@ -271,25 +277,55 @@ u8 ast_get_dp501_max_clk(struct drm_device *dev)
 	struct ast_private *ast = to_ast_private(dev);
 	u32 boot_address, offset, data;
 	u8 linkcap[4], linkrate, linklanes, maxclk = 0xff;
+	u32 *plinkcap;

-	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 = AST_DP501_GBL_VERSION;
+		data = ast_mindwm(ast, boot_address + offset);
+		if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1) /* version: 1x */
+			return maxclk;
+
+		/* Read Link Capability */
+		offset  = AST_DP501_LINKRATE;
+		plinkcap = (u32 *)linkcap;
+		*plinkcap  = 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;
+		}
+	} else {
+		if (!ast->dp501_fw_buf)
+			return AST_DP501_DEFAULT_DCLK;	/* 1024x768 as default */
+
+		/* dummy read */
+		offset = 0x0000;
+		data = readl(ast->dp501_fw_buf + offset);
+
+		/* validate FW version */
+		offset = AST_DP501_GBL_VERSION;
+		data = readl(ast->dp501_fw_buf + offset);
+		if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1) /* version: 1x */
+			return maxclk;
+
+		/* Read Link Capability */
+		offset = AST_DP501_LINKRATE;
+		plinkcap = (u32 *)linkcap;
+		*plinkcap = readl(ast->dp501_fw_buf + 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;
 }
@@ -298,26 +334,57 @@ 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;
+	u32 *pEDIDidx;

-	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 = AST_DP501_GBL_VERSION;
+		data = ast_mindwm(ast, boot_address + offset);
+		if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1)
+			return false;
+
+		/* validate PnP Monitor */
+		offset = AST_DP501_PNPMONITOR;
+		data = ast_mindwm(ast, boot_address + offset);
+		if (!(data & AST_DP501_PNP_CONNECTED))
+			return false;
+
+		/* Read EDID */
+		offset = AST_DP501_EDID_DATA;
+		for (i = 0; i < 128; i += 4) {
+			data = ast_mindwm(ast, boot_address + offset + i);
+			pEDIDidx = (u32 *)(ediddata + i);
+			*pEDIDidx = data;
+		}
+	} else {
+		if (!ast->dp501_fw_buf)
+			return false;
+
+		/* dummy read */
+		offset = 0x0000;
+		data = readl(ast->dp501_fw_buf + offset);
+
+		/* validate FW version */
+		offset = AST_DP501_GBL_VERSION;
+		data = readl(ast->dp501_fw_buf + offset);
+		if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1)
+			return false;
+
+		/* validate PnP Monitor */
+		offset = AST_DP501_PNPMONITOR;
+		data = readl(ast->dp501_fw_buf + offset);
+		if (!(data & AST_DP501_PNP_CONNECTED))
+			return false;
+
+		/* Read EDID */
+		offset = AST_DP501_EDID_DATA;
+		for (i = 0; i < 128; i += 4) {
+			data = readl(ast->dp501_fw_buf + offset + i);
+			pEDIDidx = (u32 *)(ediddata + i);
+			*pEDIDidx = data;
+		}
 	}

 	return true;
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 6b9e3b94a712..da6dfb677540 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -121,6 +121,7 @@ struct ast_private {

 	void __iomem *regs;
 	void __iomem *ioregs;
+	void __iomem *dp501_fw_buf;

 	enum ast_chip chip;
 	bool vga2_clone;
@@ -299,6 +300,17 @@ int ast_mode_config_init(struct ast_private *ast);
 #define AST_MM_ALIGN_SHIFT 4
 #define AST_MM_ALIGN_MASK ((1 << AST_MM_ALIGN_SHIFT) - 1)

+#define AST_DP501_FW_VERSION_MASK	GENMASK(7, 4)
+#define AST_DP501_FW_VERSION_1		BIT(4)
+#define AST_DP501_PNP_CONNECTED		BIT(1)
+
+#define AST_DP501_DEFAULT_DCLK	65
+
+#define AST_DP501_GBL_VERSION	0xf000
+#define AST_DP501_PNPMONITOR	0xf010
+#define AST_DP501_LINKRATE	0xf014
+#define AST_DP501_EDID_DATA	0xf020
+
 int ast_mm_init(struct ast_private *ast);

 /* ast post */
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 4ec6884f6c65..3775fe26f792 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -449,6 +449,14 @@ struct ast_private *ast_device_create(struct drm_driver *drv,
 	if (ret)
 		return ERR_PTR(ret);

+	/* map reserved buffer */
+	ast->dp501_fw_buf = NULL;
+	if (dev->vram_mm->vram_size < pci_resource_len(dev->pdev, 0)) {
+		ast->dp501_fw_buf = pci_iomap_range(dev->pdev, 0, dev->vram_mm->vram_size, 0);
+		if (!ast->dp501_fw_buf)
+			drm_info(dev, "failed to map reserved buffer!\n");
+	}
+
 	ret = ast_mode_config_init(ast);
 	if (ret)
 		return ERR_PTR(ret);
--
2.18.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH V3] drm/ast: Fixed CVE for DP501
  2020-12-11 18:52       ` kernel test robot
@ 2020-12-28  3:08         ` KuoHsiang Chou
  -1 siblings, 0 replies; 15+ messages in thread
From: KuoHsiang Chou @ 2020-12-28  3:08 UTC (permalink / raw)
  To: tzimmermann, dri-devel, linux-kernel
  Cc: airlied, airlied, daniel, jenmin_yuan, kuohsiang_chou, arc_sung,
	tommy_huang

[Bug][DP501]
If ASPEED P2A (PCI to AHB) bridge is disabled and disallowed for
CVE_2019_6260 item3, and then the monitor's EDID is unable read through
Parade DP501.
The reason is the DP501's FW is mapped to BMC addressing space rather
than Host addressing space.
The resolution is that using "pci_iomap_range()" maps to DP501's FW that
stored on the end of FB (Frame Buffer).
0In this case, FrameBuffer reserves the last 2MB used for the image of
DP501.

Signed-off-by: KuoHsiang Chou <kuohsiang_chou@aspeedtech.com>
Reported-by: kernel test robot <lkp@intel.com>
---
 drivers/gpu/drm/ast/ast_dp501.c | 139 +++++++++++++++++++++++---------
 drivers/gpu/drm/ast/ast_drv.h   |  12 +++
 drivers/gpu/drm/ast/ast_main.c  |   8 ++
 3 files changed, 123 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c
index 88121c0e0d05..cd93c44f2662 100644
--- a/drivers/gpu/drm/ast/ast_dp501.c
+++ b/drivers/gpu/drm/ast/ast_dp501.c
@@ -189,6 +189,9 @@ 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;
+
 	data = ast_mindwm(ast, 0x1e6e2100) & 0x01;
 	if (data) {
 		boot_address = get_fw_base(ast);
@@ -207,6 +210,9 @@ static bool ast_launch_m68k(struct drm_device *dev)
 	u8 *fw_addr = NULL;
 	u8 jreg;

+	if (ast->config_mode != ast_use_p2a)
+		return false;
+
 	data = ast_mindwm(ast, 0x1e6e2100) & 0x01;
 	if (!data) {

@@ -271,25 +277,55 @@ u8 ast_get_dp501_max_clk(struct drm_device *dev)
 	struct ast_private *ast = to_ast_private(dev);
 	u32 boot_address, offset, data;
 	u8 linkcap[4], linkrate, linklanes, maxclk = 0xff;
+	u32 *plinkcap;

-	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 = AST_DP501_GBL_VERSION;
+		data = ast_mindwm(ast, boot_address + offset);
+		if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1) /* version: 1x */
+			return maxclk;
+
+		/* Read Link Capability */
+		offset  = AST_DP501_LINKRATE;
+		plinkcap = (u32 *)linkcap;
+		*plinkcap  = 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;
+		}
+	} else {
+		if (!ast->dp501_fw_buf)
+			return AST_DP501_DEFAULT_DCLK;	/* 1024x768 as default */
+
+		/* dummy read */
+		offset = 0x0000;
+		data = readl(ast->dp501_fw_buf + offset);
+
+		/* validate FW version */
+		offset = AST_DP501_GBL_VERSION;
+		data = readl(ast->dp501_fw_buf + offset);
+		if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1) /* version: 1x */
+			return maxclk;
+
+		/* Read Link Capability */
+		offset = AST_DP501_LINKRATE;
+		plinkcap = (u32 *)linkcap;
+		*plinkcap = readl(ast->dp501_fw_buf + 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;
 }
@@ -298,26 +334,57 @@ 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;
+	u32 *pEDIDidx;

-	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 = AST_DP501_GBL_VERSION;
+		data = ast_mindwm(ast, boot_address + offset);
+		if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1)
+			return false;
+
+		/* validate PnP Monitor */
+		offset = AST_DP501_PNPMONITOR;
+		data = ast_mindwm(ast, boot_address + offset);
+		if (!(data & AST_DP501_PNP_CONNECTED))
+			return false;
+
+		/* Read EDID */
+		offset = AST_DP501_EDID_DATA;
+		for (i = 0; i < 128; i += 4) {
+			data = ast_mindwm(ast, boot_address + offset + i);
+			pEDIDidx = (u32 *)(ediddata + i);
+			*pEDIDidx = data;
+		}
+	} else {
+		if (!ast->dp501_fw_buf)
+			return false;
+
+		/* dummy read */
+		offset = 0x0000;
+		data = readl(ast->dp501_fw_buf + offset);
+
+		/* validate FW version */
+		offset = AST_DP501_GBL_VERSION;
+		data = readl(ast->dp501_fw_buf + offset);
+		if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1)
+			return false;
+
+		/* validate PnP Monitor */
+		offset = AST_DP501_PNPMONITOR;
+		data = readl(ast->dp501_fw_buf + offset);
+		if (!(data & AST_DP501_PNP_CONNECTED))
+			return false;
+
+		/* Read EDID */
+		offset = AST_DP501_EDID_DATA;
+		for (i = 0; i < 128; i += 4) {
+			data = readl(ast->dp501_fw_buf + offset + i);
+			pEDIDidx = (u32 *)(ediddata + i);
+			*pEDIDidx = data;
+		}
 	}

 	return true;
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 6b9e3b94a712..da6dfb677540 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -121,6 +121,7 @@ struct ast_private {

 	void __iomem *regs;
 	void __iomem *ioregs;
+	void __iomem *dp501_fw_buf;

 	enum ast_chip chip;
 	bool vga2_clone;
@@ -299,6 +300,17 @@ int ast_mode_config_init(struct ast_private *ast);
 #define AST_MM_ALIGN_SHIFT 4
 #define AST_MM_ALIGN_MASK ((1 << AST_MM_ALIGN_SHIFT) - 1)

+#define AST_DP501_FW_VERSION_MASK	GENMASK(7, 4)
+#define AST_DP501_FW_VERSION_1		BIT(4)
+#define AST_DP501_PNP_CONNECTED		BIT(1)
+
+#define AST_DP501_DEFAULT_DCLK	65
+
+#define AST_DP501_GBL_VERSION	0xf000
+#define AST_DP501_PNPMONITOR	0xf010
+#define AST_DP501_LINKRATE	0xf014
+#define AST_DP501_EDID_DATA	0xf020
+
 int ast_mm_init(struct ast_private *ast);

 /* ast post */
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 4ec6884f6c65..3775fe26f792 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -449,6 +449,14 @@ struct ast_private *ast_device_create(struct drm_driver *drv,
 	if (ret)
 		return ERR_PTR(ret);

+	/* map reserved buffer */
+	ast->dp501_fw_buf = NULL;
+	if (dev->vram_mm->vram_size < pci_resource_len(dev->pdev, 0)) {
+		ast->dp501_fw_buf = pci_iomap_range(dev->pdev, 0, dev->vram_mm->vram_size, 0);
+		if (!ast->dp501_fw_buf)
+			drm_info(dev, "failed to map reserved buffer!\n");
+	}
+
 	ret = ast_mode_config_init(ast);
 	if (ret)
 		return ERR_PTR(ret);
--
2.18.4


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH V3] drm/ast: Fixed CVE for DP501
@ 2020-12-28  3:08         ` KuoHsiang Chou
  0 siblings, 0 replies; 15+ messages in thread
From: KuoHsiang Chou @ 2020-12-28  3:08 UTC (permalink / raw)
  To: tzimmermann, dri-devel, linux-kernel
  Cc: airlied, tommy_huang, jenmin_yuan, airlied, arc_sung

[Bug][DP501]
If ASPEED P2A (PCI to AHB) bridge is disabled and disallowed for
CVE_2019_6260 item3, and then the monitor's EDID is unable read through
Parade DP501.
The reason is the DP501's FW is mapped to BMC addressing space rather
than Host addressing space.
The resolution is that using "pci_iomap_range()" maps to DP501's FW that
stored on the end of FB (Frame Buffer).
0In this case, FrameBuffer reserves the last 2MB used for the image of
DP501.

Signed-off-by: KuoHsiang Chou <kuohsiang_chou@aspeedtech.com>
Reported-by: kernel test robot <lkp@intel.com>
---
 drivers/gpu/drm/ast/ast_dp501.c | 139 +++++++++++++++++++++++---------
 drivers/gpu/drm/ast/ast_drv.h   |  12 +++
 drivers/gpu/drm/ast/ast_main.c  |   8 ++
 3 files changed, 123 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c
index 88121c0e0d05..cd93c44f2662 100644
--- a/drivers/gpu/drm/ast/ast_dp501.c
+++ b/drivers/gpu/drm/ast/ast_dp501.c
@@ -189,6 +189,9 @@ 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;
+
 	data = ast_mindwm(ast, 0x1e6e2100) & 0x01;
 	if (data) {
 		boot_address = get_fw_base(ast);
@@ -207,6 +210,9 @@ static bool ast_launch_m68k(struct drm_device *dev)
 	u8 *fw_addr = NULL;
 	u8 jreg;

+	if (ast->config_mode != ast_use_p2a)
+		return false;
+
 	data = ast_mindwm(ast, 0x1e6e2100) & 0x01;
 	if (!data) {

@@ -271,25 +277,55 @@ u8 ast_get_dp501_max_clk(struct drm_device *dev)
 	struct ast_private *ast = to_ast_private(dev);
 	u32 boot_address, offset, data;
 	u8 linkcap[4], linkrate, linklanes, maxclk = 0xff;
+	u32 *plinkcap;

-	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 = AST_DP501_GBL_VERSION;
+		data = ast_mindwm(ast, boot_address + offset);
+		if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1) /* version: 1x */
+			return maxclk;
+
+		/* Read Link Capability */
+		offset  = AST_DP501_LINKRATE;
+		plinkcap = (u32 *)linkcap;
+		*plinkcap  = 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;
+		}
+	} else {
+		if (!ast->dp501_fw_buf)
+			return AST_DP501_DEFAULT_DCLK;	/* 1024x768 as default */
+
+		/* dummy read */
+		offset = 0x0000;
+		data = readl(ast->dp501_fw_buf + offset);
+
+		/* validate FW version */
+		offset = AST_DP501_GBL_VERSION;
+		data = readl(ast->dp501_fw_buf + offset);
+		if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1) /* version: 1x */
+			return maxclk;
+
+		/* Read Link Capability */
+		offset = AST_DP501_LINKRATE;
+		plinkcap = (u32 *)linkcap;
+		*plinkcap = readl(ast->dp501_fw_buf + 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;
 }
@@ -298,26 +334,57 @@ 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;
+	u32 *pEDIDidx;

-	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 = AST_DP501_GBL_VERSION;
+		data = ast_mindwm(ast, boot_address + offset);
+		if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1)
+			return false;
+
+		/* validate PnP Monitor */
+		offset = AST_DP501_PNPMONITOR;
+		data = ast_mindwm(ast, boot_address + offset);
+		if (!(data & AST_DP501_PNP_CONNECTED))
+			return false;
+
+		/* Read EDID */
+		offset = AST_DP501_EDID_DATA;
+		for (i = 0; i < 128; i += 4) {
+			data = ast_mindwm(ast, boot_address + offset + i);
+			pEDIDidx = (u32 *)(ediddata + i);
+			*pEDIDidx = data;
+		}
+	} else {
+		if (!ast->dp501_fw_buf)
+			return false;
+
+		/* dummy read */
+		offset = 0x0000;
+		data = readl(ast->dp501_fw_buf + offset);
+
+		/* validate FW version */
+		offset = AST_DP501_GBL_VERSION;
+		data = readl(ast->dp501_fw_buf + offset);
+		if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1)
+			return false;
+
+		/* validate PnP Monitor */
+		offset = AST_DP501_PNPMONITOR;
+		data = readl(ast->dp501_fw_buf + offset);
+		if (!(data & AST_DP501_PNP_CONNECTED))
+			return false;
+
+		/* Read EDID */
+		offset = AST_DP501_EDID_DATA;
+		for (i = 0; i < 128; i += 4) {
+			data = readl(ast->dp501_fw_buf + offset + i);
+			pEDIDidx = (u32 *)(ediddata + i);
+			*pEDIDidx = data;
+		}
 	}

 	return true;
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 6b9e3b94a712..da6dfb677540 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -121,6 +121,7 @@ struct ast_private {

 	void __iomem *regs;
 	void __iomem *ioregs;
+	void __iomem *dp501_fw_buf;

 	enum ast_chip chip;
 	bool vga2_clone;
@@ -299,6 +300,17 @@ int ast_mode_config_init(struct ast_private *ast);
 #define AST_MM_ALIGN_SHIFT 4
 #define AST_MM_ALIGN_MASK ((1 << AST_MM_ALIGN_SHIFT) - 1)

+#define AST_DP501_FW_VERSION_MASK	GENMASK(7, 4)
+#define AST_DP501_FW_VERSION_1		BIT(4)
+#define AST_DP501_PNP_CONNECTED		BIT(1)
+
+#define AST_DP501_DEFAULT_DCLK	65
+
+#define AST_DP501_GBL_VERSION	0xf000
+#define AST_DP501_PNPMONITOR	0xf010
+#define AST_DP501_LINKRATE	0xf014
+#define AST_DP501_EDID_DATA	0xf020
+
 int ast_mm_init(struct ast_private *ast);

 /* ast post */
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 4ec6884f6c65..3775fe26f792 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -449,6 +449,14 @@ struct ast_private *ast_device_create(struct drm_driver *drv,
 	if (ret)
 		return ERR_PTR(ret);

+	/* map reserved buffer */
+	ast->dp501_fw_buf = NULL;
+	if (dev->vram_mm->vram_size < pci_resource_len(dev->pdev, 0)) {
+		ast->dp501_fw_buf = pci_iomap_range(dev->pdev, 0, dev->vram_mm->vram_size, 0);
+		if (!ast->dp501_fw_buf)
+			drm_info(dev, "failed to map reserved buffer!\n");
+	}
+
 	ret = ast_mode_config_init(ast);
 	if (ret)
 		return ERR_PTR(ret);
--
2.18.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2020-12-28  3:09 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-25  9:09 [PATCH] drm/ast: Fixed CVE for DP501 KuoHsiang Chou
2020-11-25  9:09 ` KuoHsiang Chou
2020-11-26 12:51 ` Thomas Zimmermann
2020-11-26 12:51   ` Thomas Zimmermann
2020-11-26 14:26   ` Daniel Vetter
2020-11-26 14:26     ` Daniel Vetter
2020-12-11  8:22   ` [PATCH v2] " KuoHsiang Chou
2020-12-11  8:22     ` KuoHsiang Chou
2020-12-11 18:52     ` kernel test robot
2020-12-11 18:52       ` kernel test robot
2020-12-11 18:52       ` kernel test robot
2020-12-28  3:08       ` [PATCH V3] " KuoHsiang Chou
2020-12-28  3:08         ` KuoHsiang Chou
2020-12-28  3:06   ` KuoHsiang Chou
2020-12-28  3:06     ` KuoHsiang Chou

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.