Re: [Bug Report] soc/aspeed: integer error in aspeed_p2a_region_acquire

[Andrew Jeffery]

Hi Changming Liu,

I've added Patrick to the thread, who authored the driver.

On Mon, 30 Mar 2020, at 06:07, Changming Liu wrote:
> Hi Joel and Andrew,
> 
> Greetings, I'm a first year PhD student who is interested in the usage 
> of UBSan in the linux kernel, and with some experiments I found that in 
> drivers/soc/aspeed/aspeed-p2a-ctrl.c function 
> aspeed_p2a_region_acquire, there is an unsigned integer error which 
> might cause unexpected behavior.
> 
> More specifically, the map structure, after the execution of 
> copy_from_user at line 180 in function aspeed_p2a_ioctl, is filled with 
> data from user space.  So the code at line 136 that is
> 
> end = map->addr + (map->length - 1);
> 
> the subtraction could underflow when map->length equals zero, also, 
> this sum could overflow. As a consequence, the check at line 149 could 
> be bypassed and the following code could be executed.
> 
> Although the fact that map->addr is a 64-bit unsigned integer and 
> map->length is 32-bit makes the overflow less likely to happen, it 
> seems doesn't eliminate the possibility entirely. I guess a access_ok 
> could do?
> 
> Due to the lack of knowledge of the interaction between this module and 
> the user space, I'm not able to assess if this is security-related 
> problem. I'd appreciate it very much to hear your valuable opinion on 
> why this could not cause any trouble if it's indeed the case, this will 
> help me under linux and UBSAN a lot! and I'm more than happy to provide 
> more information if needed.

It's certainly not expected behaviour and should be fixed :) We need to check
if the `end` calculation overflowed, but not using `access_ok()`, as the value of
`end` is an address in the physical address space of the SoC.

The current behaviour does have a security impact, though probably not in
the way you're expecting, as the driver itself is a means to violate the SoC's
security. The SoC is a BMC and exposes several PCIe devices to its host
(a VGA graphics device and a "BMC" device). Both devices expose
functionality that allows the host to perform arbitrary reads and writes to the
BMC's physical address space _if_ the BMC has allowed it to do so. This
driver controls whether the capability is exposed to the host (disabling
denies the read capability) and what regions of the SoC's physical address
space can be written. Regions are roughly broken up into memory-mapped
flash, BMC MMIO, BMC RAM and BMC LPC host controller.

Practically, enabling any region for write is to some degree unsafe: for instance
exposing the BMC's RAM to writes doesn't in any way restrict what areas of RAM
can be written, allowing e.g. arbitrary code injection into the kernel or running
processes on the BMC. Enabling writes to the BMC MMIO region allows arbitrary
control of the BMC and its peripherals without having to gain code-execution
(including escalating by removing write protection for other regions).

The driver exists to assist a trusted firmware update process used on some
platforms where the host can request (e.g. via IPMI) that BMC RAM be made
writable, then drop its firmware update payload into a predetermined location
in physical memory, and finally complete the transfer by requesting that RAM
region be returned to at least read-only mode.

To unlock unexpected regions of the BMC's address space in this scenario the
host would also have to exploit IPMI to craft an ioctl() payload with the properties
to trigger the overflow. Given that it already has complete write access to RAM it
may be easier to just exploit the BMC kernel directly rather than chain an exploit
through at least one other userspace process.

Anyway, enough background :) Thanks for the bug report and for analyzing the
driver. Hopefully Patrick can take a look and cook up a fix.

Andrew