[PATCH] hw/net/spapr_llan: validate RxBufPool fields on migration load

Chinmay Rath posted 1 patch 1 week, 1 day ago
hw/net/spapr_llan.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
[PATCH] hw/net/spapr_llan: validate RxBufPool fields on migration load
Posted by Chinmay Rath 1 week, 1 day ago
vmstate_rx_buffer_pool restores count and bufsize as raw int32 values
with no bounds checking.  A crafted migration stream can set count to
any int32 value.  spapr_vlan_get_rx_bd_from_pool() then passes the
count > 0 guard, decrements count, and uses the result as an array
index into bds[], a 4096-entry fixed heap allocation:

    dev->rx_pool[pool]->count--;
    bd  = dev->rx_pool[pool]->bds[dev->rx_pool[pool]->count];  /* OOB read  */
    dev->rx_pool[pool]->bds[dev->rx_pool[pool]->count] = 0;    /* OOB write */

With count == INT32_MAX this lands ~17 GiB past the start of a 32 KiB
heap object.  Additionally, a negative bufsize silently compares as a
huge value when widened to size_t, defeating the bufsize >= size + 8
pool-selection guard.

The hypercall path (spapr_vlan_add_rxbuf_to_pool) already enforces
0 <= count < RX_POOL_MAX_BDS; add a post_load hook to
vmstate_rx_buffer_pool that rejects any stream violating the same
invariant, failing the incoming migration.

Closes: https://gitlab.com/qemu-project/qemu/-/work_items/4147
Reported-by: Lazymio <mio@lazym.io>
Signed-off-by: Chinmay Rath <rathc@linux.ibm.com>
---
 hw/net/spapr_llan.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/hw/net/spapr_llan.c b/hw/net/spapr_llan.c
index 550848307d..a5b8906849 100644
--- a/hw/net/spapr_llan.c
+++ b/hw/net/spapr_llan.c
@@ -800,11 +800,29 @@ static bool spapr_vlan_rx_buffer_pools_needed(void *opaque)
     return (dev->compat_flags & SPAPRVLAN_FLAG_RX_BUF_POOLS) != 0;
 }
 
+static int spapr_vlan_rx_buffer_pool_post_load(void *opaque, int version_id)
+{
+    RxBufPool *rxp = opaque;
+
+    if (rxp->count < 0 || rxp->count > RX_POOL_MAX_BDS) {
+        return -EINVAL;
+    }
+    /*
+     * bufsize is compared against a size_t in spapr_vlan_get_rx_bd_from_pool(),
+     * so a negative value would compare as huge.
+     */
+    if (rxp->bufsize < 0) {
+        return -EINVAL;
+    }
+    return 0;
+}
+
 static const VMStateDescription vmstate_rx_buffer_pool = {
     .name = "spapr_llan/rx_buffer_pool",
     .version_id = 1,
     .minimum_version_id = 1,
     .needed = spapr_vlan_rx_buffer_pools_needed,
+    .post_load = spapr_vlan_rx_buffer_pool_post_load,
     .fields = (const VMStateField[]) {
         VMSTATE_INT32(bufsize, RxBufPool),
         VMSTATE_INT32(count, RxBufPool),
-- 
2.55.0