Re: [PATCH] drm/ast: Fixed CVE for DP501

From: Thomas Zimmermann
Date: Thu Nov 26 2020 - 07:51:21 EST

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@xxxxxxxxxxxxxxxxxxxxx
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: OpenPGP_0x680DC11D530B7A23.asc
Description: application/pgp-keys

Attachment: OpenPGP_signature
Description: OpenPGP digital signature