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

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

[Changming Liu]

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.

Looking forward to your valued response!

Changming Liu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Patrick Venture]

On Sun, Mar 29, 2020 at 5:37 PM Andrew Jeffery <andrew@aj.id.au> wrote:
>
> 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?

I'll take a look, but certainly adding a 32-bit value to a 64-bit
value has the possibility of overflow given a contrived attack
scenario.

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

Yeah, this driver was deliberately written to enable accessing the
memory regions controlled by the BMC, opening a security hole in the
BMC.  It's part of the design.

>
> Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[Changming Liu]

> From: Patrick Venture <venture@google.com>
> Sent: Monday, March 30, 2020 10:43 AM
> To: Andrew Jeffery <andrew@aj.id.au>
> Cc: Changming Liu <liu.changm@northeastern.edu>; Joel Stanley
> <joel@jms.id.au>; yaohway@gmail.com; linux-arm-kernel@lists.infradead.org;
> linux-aspeed@lists.ozlabs.org; Lu, Long <l.lu@northeastern.edu>
> Subject: Re: [Bug Report] soc/aspeed: integer error in
> aspeed_p2a_region_acquire
> 
> On Sun, Mar 29, 2020 at 5:37 PM Andrew Jeffery <andrew@aj.id.au> wrote:
> >
> > 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?
> 
> I'll take a look, but certainly adding a 32-bit value to a 64-bit value has the
> possibility of overflow given a contrived attack scenario.
> 
> > >
> > > 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.
> 
Hi Andrew and Patrick,
Thank you so much for this vivid enlightening explanation! The driver code has never been this clear.
Although it was a lot for me to digest but it undoubtedly helped a lot to understand how linux drivers actually worked.
You guys are the hero. : )

Changming Liu

> Yeah, this driver was deliberately written to enable accessing the memory
> regions controlled by the BMC, opening a security hole in the BMC.  It's part of
> the design.
> 
> >
> > Andrew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel