Re: [PATCH] firmware: google: memconsole: Prevent overrun attack on coreboot console

[Julius Werner]

Sorry. Resent.

Re: [PATCH] firmware: google: memconsole: Prevent overrun attack on coreboot console

[Julius Werner]

Sorry. Resent.

Re: [PATCH] firmware: google: memconsole: Prevent overrun attack on coreboot console

[Greg Kroah-Hartman]

A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?


http://daringfireball.net/2007/07/on_top

On Mon, May 22, 2017 at 06:36:44PM -0700, Julius Werner wrote:
> Fixes: a5061d028 ("firmware: google: memconsole: Adapt to new coreboot
> ring buffer format")

Ok, can you resend and add this to the patch so I don't have to manually
do it?

thanks,

greg k-h

Re: [PATCH] firmware: google: memconsole: Prevent overrun attack on coreboot console

[Greg Kroah-Hartman]

A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?


http://daringfireball.net/2007/07/on_top

On Mon, May 22, 2017 at 06:36:44PM -0700, Julius Werner wrote:
> Fixes: a5061d028 ("firmware: google: memconsole: Adapt to new coreboot
> ring buffer format")

Ok, can you resend and add this to the patch so I don't have to manually
do it?

thanks,

greg k-h

Re: [PATCH] firmware: google: memconsole: Prevent overrun attack on coreboot console

[Julius Werner]

Fixes: a5061d028 ("firmware: google: memconsole: Adapt to new coreboot
ring buffer format")

On Sat, May 20, 2017 at 1:32 AM, Greg Kroah-Hartman
 wrote:
> On Fri, May 19, 2017 at 02:44:38PM -0700, Julius Werner wrote:
>> The recent coreboot memory console update (firmware: google: memconsole:
>> Adapt to new coreboot ring buffer format) introduced a small security
>> issue in the driver: The new driver implementation parses the memory
>> console structure again on every access. This is intentional so that
>> additional lines added concurrently by runtime firmware can be read out.
>>
>> However, if an attacker can write to the structure, they could increase
>> the size value to a point where the driver would read potentially
>> sensitive memory areas from outside the original console buffer during
>> the next access. This can be done through /dev/mem, since the console
>> buffer usually resides in firmware-reserved memory that is not covered
>> by STRICT_DEVMEM.
>>
>> This patch resolves that problem by reading the buffer's size value only
>> once during boot (where we can still trust the structure). Other parts
>> of the structure can still be modified at runtime, but the driver's
>> bounds checks make sure that it will never read outside the buffer.
>>
>> Signed-off-by: Julius Werner 
>
> Care to provide a "Fixes: SHA" type line here saying what commit this
> fixes the issue for, to allow people to know what is going on.
>
> thanks,
>
> greg k-h

Re: [PATCH] firmware: google: memconsole: Prevent overrun attack on coreboot console

[Julius Werner]

Fixes: a5061d028 ("firmware: google: memconsole: Adapt to new coreboot
ring buffer format")

On Sat, May 20, 2017 at 1:32 AM, Greg Kroah-Hartman
 wrote:
> On Fri, May 19, 2017 at 02:44:38PM -0700, Julius Werner wrote:
>> The recent coreboot memory console update (firmware: google: memconsole:
>> Adapt to new coreboot ring buffer format) introduced a small security
>> issue in the driver: The new driver implementation parses the memory
>> console structure again on every access. This is intentional so that
>> additional lines added concurrently by runtime firmware can be read out.
>>
>> However, if an attacker can write to the structure, they could increase
>> the size value to a point where the driver would read potentially
>> sensitive memory areas from outside the original console buffer during
>> the next access. This can be done through /dev/mem, since the console
>> buffer usually resides in firmware-reserved memory that is not covered
>> by STRICT_DEVMEM.
>>
>> This patch resolves that problem by reading the buffer's size value only
>> once during boot (where we can still trust the structure). Other parts
>> of the structure can still be modified at runtime, but the driver's
>> bounds checks make sure that it will never read outside the buffer.
>>
>> Signed-off-by: Julius Werner 
>
> Care to provide a "Fixes: SHA" type line here saying what commit this
> fixes the issue for, to allow people to know what is going on.
>
> thanks,
>
> greg k-h

Re: [PATCH] firmware: google: memconsole: Prevent overrun attack on coreboot console

[Greg Kroah-Hartman]

On Fri, May 19, 2017 at 02:44:38PM -0700, Julius Werner wrote:
> The recent coreboot memory console update (firmware: google: memconsole:
> Adapt to new coreboot ring buffer format) introduced a small security
> issue in the driver: The new driver implementation parses the memory
> console structure again on every access. This is intentional so that
> additional lines added concurrently by runtime firmware can be read out.
> 
> However, if an attacker can write to the structure, they could increase
> the size value to a point where the driver would read potentially
> sensitive memory areas from outside the original console buffer during
> the next access. This can be done through /dev/mem, since the console
> buffer usually resides in firmware-reserved memory that is not covered
> by STRICT_DEVMEM.
> 
> This patch resolves that problem by reading the buffer's size value only
> once during boot (where we can still trust the structure). Other parts
> of the structure can still be modified at runtime, but the driver's
> bounds checks make sure that it will never read outside the buffer.
> 
> Signed-off-by: Julius Werner 

Care to provide a "Fixes: SHA" type line here saying what commit this
fixes the issue for, to allow people to know what is going on.

thanks,

greg k-h

Re: [PATCH] firmware: google: memconsole: Prevent overrun attack on coreboot console

[Greg Kroah-Hartman]

On Fri, May 19, 2017 at 02:44:38PM -0700, Julius Werner wrote:
> The recent coreboot memory console update (firmware: google: memconsole:
> Adapt to new coreboot ring buffer format) introduced a small security
> issue in the driver: The new driver implementation parses the memory
> console structure again on every access. This is intentional so that
> additional lines added concurrently by runtime firmware can be read out.
> 
> However, if an attacker can write to the structure, they could increase
> the size value to a point where the driver would read potentially
> sensitive memory areas from outside the original console buffer during
> the next access. This can be done through /dev/mem, since the console
> buffer usually resides in firmware-reserved memory that is not covered
> by STRICT_DEVMEM.
> 
> This patch resolves that problem by reading the buffer's size value only
> once during boot (where we can still trust the structure). Other parts
> of the structure can still be modified at runtime, but the driver's
> bounds checks make sure that it will never read outside the buffer.
> 
> Signed-off-by: Julius Werner 

Care to provide a "Fixes: SHA" type line here saying what commit this
fixes the issue for, to allow people to know what is going on.

thanks,

greg k-h

[PATCH] firmware: google: memconsole: Prevent overrun attack on coreboot console

[Julius Werner]

The recent coreboot memory console update (firmware: google: memconsole:
Adapt to new coreboot ring buffer format) introduced a small security
issue in the driver: The new driver implementation parses the memory
console structure again on every access. This is intentional so that
additional lines added concurrently by runtime firmware can be read out.

However, if an attacker can write to the structure, they could increase
the size value to a point where the driver would read potentially
sensitive memory areas from outside the original console buffer during
the next access. This can be done through /dev/mem, since the console
buffer usually resides in firmware-reserved memory that is not covered
by STRICT_DEVMEM.

This patch resolves that problem by reading the buffer's size value only
once during boot (where we can still trust the structure). Other parts
of the structure can still be modified at runtime, but the driver's
bounds checks make sure that it will never read outside the buffer.

Signed-off-by: Julius Werner 
---
 drivers/firmware/google/memconsole-coreboot.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/google/memconsole-coreboot.c 
b/drivers/firmware/google/memconsole-coreboot.c
index 7d39f4ef5d9e..52738887735c 100644
--- a/drivers/firmware/google/memconsole-coreboot.c
+++ b/drivers/firmware/google/memconsole-coreboot.c
@@ -26,7 +26,7 @@
 
 /* CBMEM firmware console log descriptor. */
 struct cbmem_cons {
-   u32 size;
+   u32 size_dont_access_after_boot;
u32 cursor;
u8  body[0];
 } __packed;
@@ -35,6 +35,7 @@ struct cbmem_cons {
 #define OVERFLOW (1 << 31)
 
 static struct cbmem_cons __iomem *cbmem_console;
+static u32 cbmem_console_size;
 
 /*
  * The cbmem_console structure is read again on every access because it may
@@ -47,7 +48,7 @@ static ssize_t memconsole_coreboot_read(char *buf, loff_t 
pos, size_t count)
 {
u32 cursor = cbmem_console->cursor & CURSOR_MASK;
u32 flags = cbmem_console->cursor & ~CURSOR_MASK;
-   u32 size = cbmem_console->size;
+   u32 size = cbmem_console_size;
struct seg {/* describes ring buffer segments in logical order */
u32 phys;   /* physical offset from start of mem buffer */
u32 len;/* length of segment */
@@ -81,8 +82,10 @@ static int memconsole_coreboot_init(phys_addr_t physaddr)
if (!tmp_cbmc)
return -ENOMEM;
 
+   /* Read size only once to prevent overrun attack through /dev/mem. */
+   cbmem_console_size = tmp_cbmc->size_dont_access_after_boot;
cbmem_console = memremap(physaddr,
-tmp_cbmc->size + sizeof(*cbmem_console),
+cbmem_console_size + sizeof(*cbmem_console),
 MEMREMAP_WB);
memunmap(tmp_cbmc);
 
-- 
2.12.2

[PATCH] firmware: google: memconsole: Prevent overrun attack on coreboot console

[Julius Werner]

The recent coreboot memory console update (firmware: google: memconsole:
Adapt to new coreboot ring buffer format) introduced a small security
issue in the driver: The new driver implementation parses the memory
console structure again on every access. This is intentional so that
additional lines added concurrently by runtime firmware can be read out.

However, if an attacker can write to the structure, they could increase
the size value to a point where the driver would read potentially
sensitive memory areas from outside the original console buffer during
the next access. This can be done through /dev/mem, since the console
buffer usually resides in firmware-reserved memory that is not covered
by STRICT_DEVMEM.

This patch resolves that problem by reading the buffer's size value only
once during boot (where we can still trust the structure). Other parts
of the structure can still be modified at runtime, but the driver's
bounds checks make sure that it will never read outside the buffer.

Signed-off-by: Julius Werner 
---
 drivers/firmware/google/memconsole-coreboot.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/google/memconsole-coreboot.c 
b/drivers/firmware/google/memconsole-coreboot.c
index 7d39f4ef5d9e..52738887735c 100644
--- a/drivers/firmware/google/memconsole-coreboot.c
+++ b/drivers/firmware/google/memconsole-coreboot.c
@@ -26,7 +26,7 @@
 
 /* CBMEM firmware console log descriptor. */
 struct cbmem_cons {
-   u32 size;
+   u32 size_dont_access_after_boot;
u32 cursor;
u8  body[0];
 } __packed;
@@ -35,6 +35,7 @@ struct cbmem_cons {
 #define OVERFLOW (1 << 31)
 
 static struct cbmem_cons __iomem *cbmem_console;
+static u32 cbmem_console_size;
 
 /*
  * The cbmem_console structure is read again on every access because it may
@@ -47,7 +48,7 @@ static ssize_t memconsole_coreboot_read(char *buf, loff_t 
pos, size_t count)
 {
u32 cursor = cbmem_console->cursor & CURSOR_MASK;
u32 flags = cbmem_console->cursor & ~CURSOR_MASK;
-   u32 size = cbmem_console->size;
+   u32 size = cbmem_console_size;
struct seg {/* describes ring buffer segments in logical order */
u32 phys;   /* physical offset from start of mem buffer */
u32 len;/* length of segment */
@@ -81,8 +82,10 @@ static int memconsole_coreboot_init(phys_addr_t physaddr)
if (!tmp_cbmc)
return -ENOMEM;
 
+   /* Read size only once to prevent overrun attack through /dev/mem. */
+   cbmem_console_size = tmp_cbmc->size_dont_access_after_boot;
cbmem_console = memremap(physaddr,
-tmp_cbmc->size + sizeof(*cbmem_console),
+cbmem_console_size + sizeof(*cbmem_console),
 MEMREMAP_WB);
memunmap(tmp_cbmc);
 
-- 
2.12.2