diff mbox

[2/2] seabios: pciinit: fix overflow when bar allocation.

Message ID 467b97ca8eab5336ed7fa6fae93db3695a04e8dd.1280120278.git.yamahata@valinux.co.jp
State New
Headers show

Commit Message

Isaku Yamahata July 26, 2010, 5:02 a.m. UTC 
When allocating bar, overflow can occur.
So add overflow check and don't allocate bar if overflowed.
Overflow check is ugly, but necessary.
Another suggested way is to change related variables u64 from u32
thus overflow can't occur because the related value are all u32 addressable.
Anyway even with u64, it is necessary to the resulted value > max_u32.

Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
---
 src/pciinit.c |   48 ++++++++++++++++++++++++++++++++++--------------
 1 files changed, 34 insertions(+), 14 deletions(-)

Comments

Kevin O'Connor July 27, 2010, 1:45 a.m. UTC | #1 
On Mon, Jul 26, 2010 at 02:02:46PM +0900, Isaku Yamahata wrote:
> When allocating bar, overflow can occur.
> So add overflow check and don't allocate bar if overflowed.
> Overflow check is ugly, but necessary.
> Another suggested way is to change related variables u64 from u32
> thus overflow can't occur because the related value are all u32 addressable.
> Anyway even with u64, it is necessary to the resulted value > max_u32.

I wonder if we could create malloc zones for this and then use
pmm_alloc.  The pmm_alloc function already handles overflow and
alignments.

One difficulty, though, is that pmm_alloc doesn't guarentee linear
allocations, and it looks like pci_bios_init_device_bridge assumes
this.

-Kevin
Isaku Yamahata July 27, 2010, 9:11 a.m. UTC | #2 
On Mon, Jul 26, 2010 at 09:45:42PM -0400, Kevin O'Connor wrote:
> On Mon, Jul 26, 2010 at 02:02:46PM +0900, Isaku Yamahata wrote:
> > When allocating bar, overflow can occur.
> > So add overflow check and don't allocate bar if overflowed.
> > Overflow check is ugly, but necessary.
> > Another suggested way is to change related variables u64 from u32
> > thus overflow can't occur because the related value are all u32 addressable.
> > Anyway even with u64, it is necessary to the resulted value > max_u32.
> 
> I wonder if we could create malloc zones for this and then use
> pmm_alloc.  The pmm_alloc function already handles overflow and
> alignments.
> 
> One difficulty, though, is that pmm_alloc doesn't guarentee linear
> allocations, and it looks like pci_bios_init_device_bridge assumes
> this.

Yes, contiguous allocation is necessary for bridge filtering.
Anyway I'll give pmm_malloc() a try and see what can be done.
diff mbox

Patch

diff --git a/src/pciinit.c b/src/pciinit.c
index f75e552..9be895c 100644
--- a/src/pciinit.c
+++ b/src/pciinit.c
@@ -49,6 +49,12 @@  static void pci_set_io_region_addr(u16 bdf, int region_num, u32 addr)
     dprintf(1, "region %d: 0x%08x\n", region_num, addr);
 }
 
+static inline int pci_bar_overflow(u32 addr, u32 size)
+{
+    return (ALIGN(addr, size) < addr) ||
+        (ALIGN(addr, size) + size < ALIGN(addr, size));
+}
+
 /*
  * return value
  *      0:     32bit BAR
@@ -78,7 +84,8 @@  static int pci_bios_allocate_region(u16 bdf, int region_num)
     if (val != 0) {
         if (val & PCI_BASE_ADDRESS_SPACE_IO) {
             paddr = &pci_bios_io_addr;
-            if (ALIGN(*paddr, size) + size >= 64 * 1024) {
+            if (pci_bar_overflow(*paddr, size) ||
+                ALIGN(*paddr, size) + size >= 64 * 1024) {
                 dprintf(1,
                         "io region of (bdf 0x%x bar %d) can't be mapped.\n",
                         bdf, region_num);
@@ -90,7 +97,8 @@  static int pci_bios_allocate_region(u16 bdf, int region_num)
                  /* If pci_bios_prefmem_addr == 0, keep old behaviour */
                  pci_bios_prefmem_addr != 0) {
             paddr = &pci_bios_prefmem_addr;
-            if (ALIGN(*paddr, size) + size >= BUILD_PCIPREFMEM_END) {
+            if (pci_bar_overflow(*paddr, size) ||
+                ALIGN(*paddr, size) + size >= BUILD_PCIPREFMEM_END) {
                 dprintf(1,
                         "prefmem region of (bdf 0x%x bar %d) can't be mapped. "
                         "decrease BUILD_PCIMEM_SIZE and recompile. size %x\n",
@@ -99,7 +107,8 @@  static int pci_bios_allocate_region(u16 bdf, int region_num)
             }
         } else {
             paddr = &pci_bios_mem_addr;
-            if (ALIGN(*paddr, size) + size >= BUILD_PCIMEM_END) {
+            if (pci_bar_overflow(*paddr, size) ||
+                ALIGN(*paddr, size) + size >= BUILD_PCIMEM_END) {
                 dprintf(1,
                         "mem region of (bdf 0x%x bar %d) can't be mapped. "
                         "increase BUILD_PCIMEM_SIZE and recompile. size %x\n",
@@ -152,6 +161,12 @@  static const struct pci_device_id pci_isa_bridge_tbl[] = {
     PCI_DEVICE_END
 };
 
+static inline int pci_addr_overflow(u32 old, u32 base, u32 addr, u32 end)
+{
+    /* if without overflow, old <= base <= addr <= end */
+    return (old > base) || (base > addr) || (addr > end);
+}
+
 #define PCI_IO_ALIGN            4096
 #define PCI_IO_SHIFT            8
 #define PCI_MEMORY_ALIGN        (1UL << 20)
@@ -184,36 +199,41 @@  static void pci_bios_init_device_bridge(u16 bdf, void *arg)
         pci_bios_init_device_in_bus(secbus);
     }
 
-    pci_bios_io_addr = ALIGN(pci_bios_io_addr, PCI_IO_ALIGN);
-    pci_bios_mem_addr = ALIGN(pci_bios_mem_addr, PCI_MEMORY_ALIGN);
-    pci_bios_prefmem_addr =
-        ALIGN(pci_bios_prefmem_addr, PCI_PREF_MEMORY_ALIGN);
-
-    u32 io_end = pci_bios_io_addr;
-    if (io_end == io_base) {
+    u32 io_end = ALIGN(pci_bios_io_addr, PCI_IO_ALIGN);
+    if (pci_addr_overflow(io_old, io_base, pci_bios_io_addr, io_end) ||
+        io_end == io_base) {
         pci_bios_io_addr = io_old;
         io_base = 0xffff;
         io_end = 1;
+    } else {
+        pci_bios_io_addr = io_end;
     }
     pci_config_writeb(bdf, PCI_IO_BASE, io_base >> PCI_IO_SHIFT);
     pci_config_writew(bdf, PCI_IO_BASE_UPPER16, 0);
     pci_config_writeb(bdf, PCI_IO_LIMIT, (io_end - 1) >> PCI_IO_SHIFT);
     pci_config_writew(bdf, PCI_IO_LIMIT_UPPER16, 0);
 
-    u32 mem_end = pci_bios_mem_addr;
-    if (mem_end == mem_base) {
+    u32 mem_end = ALIGN(pci_bios_mem_addr, PCI_MEMORY_ALIGN);
+    if (pci_addr_overflow(mem_old, mem_base, pci_bios_mem_addr, mem_end) ||
+        mem_end == mem_base) {
         pci_bios_mem_addr = mem_old;
         mem_base = 0xffffffff;
         mem_end = 1;
+    } else {
+        pci_bios_io_addr = mem_end;
     }
     pci_config_writew(bdf, PCI_MEMORY_BASE, mem_base >> PCI_MEMORY_SHIFT);
     pci_config_writew(bdf, PCI_MEMORY_LIMIT, (mem_end -1) >> PCI_MEMORY_SHIFT);
 
-    u32 prefmem_end = pci_bios_prefmem_addr;
-    if (prefmem_end == prefmem_base) {
+    u32 prefmem_end = ALIGN(pci_bios_prefmem_addr, PCI_PREF_MEMORY_ALIGN);
+    if (pci_addr_overflow(prefmem_old, prefmem_base,
+                          pci_bios_prefmem_addr, prefmem_end) ||
+        prefmem_end == prefmem_base) {
         pci_bios_prefmem_addr = prefmem_old;
         prefmem_base = 0xffffffff;
         prefmem_end = 1;
+    } else {
+        pci_bios_prefmem_addr = prefmem_end;
     }
     pci_config_writew(bdf, PCI_PREF_MEMORY_BASE,
                       prefmem_base >> PCI_PREF_MEMORY_SHIFT);