[PATCH] mcb: fix a missing-check bug

[PATCH] mcb: fix a missing-check bug

[Wenwen Wang]

In chameleon_parse_cells(), to parse each cell, the descriptor type 'dtype'
is acquired from the IO memory region pointed by 'p' through readl() in
get_next_dtype(). Then 'dtype' is checked to see whether it is
CHAMELEON_DTYPE_GENERAL. If yes, chameleon_parse_gdd() is invoked to parse
Chameleon general device descriptor. In chameleon_parse_gdd(), the data in
the IO memory region is read again through readl() field by field.
Specifically, the 'reg1' field contains the type information. That means
the type is read twice. More importantly, no check is re-enforced after the
second read. Given that the IO memory region can also be accessed by the
device, it is possible that a malicious device controlled by an attacker
can modify the type information between the two reads. This can cause
undefined behavior of the kernel and introduce potential security risk.

This patch adds a necessary check after the second read to make sure the
descriptor type is CHAMELEON_DTYPE_GENERAL. Otherwise, an error code EINVAL
will be returned.

Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
 drivers/mcb/mcb-parse.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mcb/mcb-parse.c b/drivers/mcb/mcb-parse.c
index 7369bda..f01a6c7 100644
--- a/drivers/mcb/mcb-parse.c
+++ b/drivers/mcb/mcb-parse.c
@@ -51,6 +51,10 @@ static int chameleon_parse_gdd(struct mcb_bus *bus,
 		return -ENOMEM;
 
 	reg1 = readl(&gdd->reg1);
+	if ((reg1 >> 28) != CHAMELEON_DTYPE_GENERAL) {
+		ret = -EINVAL;
+		goto err;
+	}
 	reg2 = readl(&gdd->reg2);
 	offset = readl(&gdd->offset);
 	size = readl(&gdd->size);
-- 
2.7.4

Re: [PATCH] mcb: fix a missing-check bug

[Johannes Thumshirn]

Hi Wenwen,

Sorry for the late reply:

On 19/10/18 17:11, Wenwen Wang wrote:
> In chameleon_parse_cells(), to parse each cell, the descriptor type 'dtype'
> is acquired from the IO memory region pointed by 'p' through readl() in
> get_next_dtype(). Then 'dtype' is checked to see whether it is
> CHAMELEON_DTYPE_GENERAL. If yes, chameleon_parse_gdd() is invoked to parse
> Chameleon general device descriptor. In chameleon_parse_gdd(), the data in
> the IO memory region is read again through readl() field by field.
> Specifically, the 'reg1' field contains the type information. That means
> the type is read twice. More importantly, no check is re-enforced after the
> second read. Given that the IO memory region can also be accessed by the
> device, it is possible that a malicious device controlled by an attacker
> can modify the type information between the two reads. This can cause
> undefined behavior of the kernel and introduce potential security risk.

Yes but this doesn't really mitigate the problem, does it? If a
malicious attacker controlling the MMIO space can change the register
contents after the first read, what stops him/her from doing it after
the second, third, 4096th read?

>  
>  	reg1 = readl(&gdd->reg1);
> +	if ((reg1 >> 28) != CHAMELEON_DTYPE_GENERAL) {
> +		ret = -EINVAL;
> +		goto err;
> +	}


Just an advice for your next submission, give that 'magic' 28 a define
(like CHAMELEON_DTYPE_SHIFT or whatever), this makes the code nicer to read.