[PATCH for 7.3] conf: Fix heap corruption when hot-adding a lease

Peter Krempa posted 1 patch 2 weeks, 2 days ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/72848f89ce29aa26f4f1c115c95d14114e8ec760.1619950384.git.pkrempa@redhat.com
src/conf/domain_conf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

[PATCH for 7.3] conf: Fix heap corruption when hot-adding a lease

Posted by Peter Krempa 2 weeks, 2 days ago
Commit 28a86993162f7d2f ( v6.9.0-179-g28a8699316 ) incorrectly replaced
VIR_EXPAND_N by g_renew.

VIR_EXPAND_N has these two extra effects apart from reallocating memory:

1) The newly allocated memory is zeroed out
2) The number of elements in the array which is passed to VIR_EXPAND_N
   is increased.

This comes into play when used with virDomainLeaseInsertPreAlloced,
which expects that the array element count already includes the space
for the added 'lease', by plainly just assigning to 'leases[nleases - 1'

Since g_renew does not increase the number of elements in the array
any existing code which calls virDomainLeaseInsertPreAlloced thus either
overwrites a lease definition or corrupts the heap if there are no
leases to start with.

To preserve existing functionality we revert the code back to using
VIR_EXPAND_N which at this point doesn't return any value, so other
commits don't need to be reverted.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1953577
Signed-off-by: Peter Krempa <pkrempa@redhat.com>
---
 src/conf/domain_conf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 9d98f487ea..84570c001c 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -16837,7 +16837,7 @@ int virDomainLeaseIndex(virDomainDef *def,

 void virDomainLeaseInsertPreAlloc(virDomainDef *def)
 {
-    def->leases = g_renew(virDomainLeaseDef *, def->leases, def->nleases + 1);
+    VIR_EXPAND_N(def->leases, def->nleases, 1);
 }

 void virDomainLeaseInsert(virDomainDef *def, virDomainLeaseDef *lease)
-- 
2.30.2

Re: [PATCH for 7.3] conf: Fix heap corruption when hot-adding a lease

Posted by Jiri Denemark 2 weeks, 1 day ago
On Sun, May 02, 2021 at 12:13:50 +0200, Peter Krempa wrote:
> Commit 28a86993162f7d2f ( v6.9.0-179-g28a8699316 ) incorrectly replaced
> VIR_EXPAND_N by g_renew.
> 
> VIR_EXPAND_N has these two extra effects apart from reallocating memory:
> 
> 1) The newly allocated memory is zeroed out
> 2) The number of elements in the array which is passed to VIR_EXPAND_N
>    is increased.
> 
> This comes into play when used with virDomainLeaseInsertPreAlloced,
> which expects that the array element count already includes the space
> for the added 'lease', by plainly just assigning to 'leases[nleases - 1'

s/1/1]/

> 
> Since g_renew does not increase the number of elements in the array
> any existing code which calls virDomainLeaseInsertPreAlloced thus either
> overwrites a lease definition or corrupts the heap if there are no
> leases to start with.
> 
> To preserve existing functionality we revert the code back to using
> VIR_EXPAND_N which at this point doesn't return any value, so other
> commits don't need to be reverted.

The second point could have been solved by passing ++def->nleases to
g_renew. But using VIR_EXPAND_N instead solves both issues and we have a
lot places with VIR_EXPAND_N so we can fix them all at some point if we
want to drop this wrapper for some reason.

Reviewed-by: Jiri Denemark <jdenemar@redhat.com>