[PATCH] virsh: Fix integer overflow in allocpages

Michal Privoznik posted 1 patch 2 years, 1 month ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/ba116b342bac3a349f08bb74c969f189e2d6b9a9.1648642754.git.mprivozn@redhat.com
tools/virsh-completer-host.c | 2 +-
tools/virsh-host.c           | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
[PATCH] virsh: Fix integer overflow in allocpages
Posted by Michal Privoznik 2 years, 1 month ago
I've came across an aarch64 system which supports hugepages up to
16GiB of size. However, I was unable to allocate them using
virsh allocpages. This is because cmdAllocpages() uses
vshCommandOptScaledInt(), which scales passed value into bytes,
but since the virNodeAllocPages() expects size in KiB the
variable holding bytes is then divided by 1024. However, the
limit for the biggest value passed to vshCommandOptScaledInt() is
UINT_MAX which is now obviously wrong, as it needs to be UINT_MAX
* 1024.

The same bug is in completer. But here, let's use ULLONG_MAX so
that we don't have to care about it anymore.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 tools/virsh-completer-host.c | 2 +-
 tools/virsh-host.c           | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/virsh-completer-host.c b/tools/virsh-completer-host.c
index 40cb687582..cbdc3f0d49 100644
--- a/tools/virsh-completer-host.c
+++ b/tools/virsh-completer-host.c
@@ -42,7 +42,7 @@ virshPagesizeNodeToString(xmlNodePtr node)
     unit = virXMLPropString(node, "unit");
     if (virStrToLong_ull(pagesize, NULL, 10, &byteval) < 0)
         return NULL;
-    if (virScaleInteger(&byteval, unit, 1024, UINT_MAX) < 0)
+    if (virScaleInteger(&byteval, unit, 1024, ULLONG_MAX) < 0)
         return NULL;
     size = vshPrettyCapacity(byteval, &suffix);
     ret = g_strdup_printf("%.0f%s", size, suffix);
diff --git a/tools/virsh-host.c b/tools/virsh-host.c
index 2e3cbc39b6..1e83d19fa1 100644
--- a/tools/virsh-host.c
+++ b/tools/virsh-host.c
@@ -488,7 +488,7 @@ cmdAllocpages(vshControl *ctl, const vshCmd *cmd)
     if (cellno && vshCommandOptInt(ctl, cmd, "cellno", &startCell) < 0)
         return false;
 
-    if (vshCommandOptScaledInt(ctl, cmd, "pagesize", &tmp, 1024, UINT_MAX) < 0)
+    if (vshCommandOptScaledInt(ctl, cmd, "pagesize", &tmp, 1024, UINT_MAX * 1024ULL) < 0)
         return false;
     pageSizes[0] = VIR_DIV_UP(tmp, 1024);
 
-- 
2.35.1
Re: [PATCH] virsh: Fix integer overflow in allocpages
Posted by Ján Tomko 2 years ago
On a Wednesday in 2022, Michal Privoznik wrote:
>I've came across an aarch64 system which supports hugepages up to
>16GiB of size. However, I was unable to allocate them using
>virsh allocpages. This is because cmdAllocpages() uses
>vshCommandOptScaledInt(), which scales passed value into bytes,
>but since the virNodeAllocPages() expects size in KiB the
>variable holding bytes is then divided by 1024. However, the
>limit for the biggest value passed to vshCommandOptScaledInt() is
>UINT_MAX which is now obviously wrong, as it needs to be UINT_MAX
>* 1024.
>
>The same bug is in completer. But here, let's use ULLONG_MAX so
>that we don't have to care about it anymore.
>
>Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>---
> tools/virsh-completer-host.c | 2 +-
> tools/virsh-host.c           | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>

Reviewed-by: Ján Tomko <jtomko@redhat.com>

Jano