[PATCH v3 0/5] pmstat interface fixes

[PATCH v3 0/5] pmstat interface fixes

[Ross Lagerwall]

Clarify and fix usage of the pmstat hypervisor and tools interfaces
regarding sizes of buffers passed in.

Ross Lagerwall (5):
  x86/pmstat: Check size of PMSTAT_get_pxstat buffers
  public/sysctl: Clarify usage of pm_{px,cx}_stat
  cpufreq: Avoid potential buffer overrun and leak
  libxc/PM: Ensure pxstat buffers are correctly sized
  libxc/PM: Retry get_pxstat if data is incomplete

 tools/libs/ctrl/xc_pm.c       | 21 +++++++-------
 tools/misc/xenpm.c            | 48 ++++++++++++++++++++-----------
 xen/drivers/acpi/pmstat.c     |  7 +++--
 xen/drivers/cpufreq/cpufreq.c |  3 +-
 xen/include/public/sysctl.h   | 54 +++++++++++++++++++++++++++--------
 5 files changed, 90 insertions(+), 43 deletions(-)

-- 
2.49.0

[PATCH v3 1/5] x86/pmstat: Check size of PMSTAT_get_pxstat buffers

[Ross Lagerwall]

Check that the total number of states passed in and hence the size of
buffers is sufficient to avoid writing more than the caller has
allocated.

The interface is not explicit about whether getpx.total is expected to
be set by the caller in this case but since it is always set in
libxenctrl it seems reasonable to check it and make it explicit.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Fixes: c06a7db0c547 ("X86 and IA64: Update cpufreq statistic logic for supporting both x86 and ia64")
---

In v3:

* Fix if condition
* Move some header comments from patch 2
* Clarify some comments

 xen/drivers/acpi/pmstat.c   |  7 +++++--
 xen/include/public/sysctl.h | 15 +++++++++++++--
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/xen/drivers/acpi/pmstat.c b/xen/drivers/acpi/pmstat.c
index c51b9ca358c2..0d570e28bf11 100644
--- a/xen/drivers/acpi/pmstat.c
+++ b/xen/drivers/acpi/pmstat.c
@@ -103,8 +103,11 @@ int do_get_pm_info(struct xen_sysctl_get_pmstat *op)
 
         cpufreq_residency_update(op->cpuid, pxpt->u.cur);
 
-        ct = pmpt->perf.state_count;
-        if ( copy_to_guest(op->u.getpx.trans_pt, pxpt->u.trans_pt, ct*ct) )
+        ct = min(pmpt->perf.state_count, op->u.getpx.total + 0U);
+
+        /* Avoid partial copying of 2-D array */
+        if ( ct == op->u.getpx.total &&
+             copy_to_guest(op->u.getpx.trans_pt, pxpt->u.trans_pt, ct * ct) )
         {
             spin_unlock(cpufreq_statistic_lock);
             ret = -EFAULT;
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 9eca72865b87..906a3364fbd9 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -215,11 +215,22 @@ typedef struct pm_px_val pm_px_val_t;
 DEFINE_XEN_GUEST_HANDLE(pm_px_val_t);
 
 struct pm_px_stat {
-    uint8_t total;        /* total Px states */
+    /*
+     * IN: Number of elements in pt, number of rows/columns in trans_pt
+     *     (PMSTAT_get_pxstat)
+     * OUT: total Px states (PMSTAT_get_max_px, PMSTAT_get_pxstat)
+     */
+    uint8_t total;
     uint8_t usable;       /* usable Px states */
     uint8_t last;         /* last Px state */
     uint8_t cur;          /* current Px state */
-    XEN_GUEST_HANDLE_64(uint64) trans_pt;   /* Px transition table */
+    /*
+     * OUT: Px transition table. This should have total * total elements.
+     *      As it is a 2-D array, this will not be copied if it is smaller than
+     *      the hypervisor's Px transition table. (PMSTAT_get_pxstat)
+     */
+    XEN_GUEST_HANDLE_64(uint64) trans_pt;
+    /* OUT: This should have total elements (PMSTAT_get_pxstat) */
     XEN_GUEST_HANDLE_64(pm_px_val_t) pt;
 };
 
-- 
2.49.0

[PATCH v3 2/5] public/sysctl: Clarify usage of pm_{px,cx}_stat

[Ross Lagerwall]

Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---

In v3:

* Moved some changes to patch 1
* Clarified some comments

 xen/include/public/sysctl.h | 39 +++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 906a3364fbd9..b1e3a48194d8 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -221,9 +221,9 @@ struct pm_px_stat {
      * OUT: total Px states (PMSTAT_get_max_px, PMSTAT_get_pxstat)
      */
     uint8_t total;
-    uint8_t usable;       /* usable Px states */
-    uint8_t last;         /* last Px state */
-    uint8_t cur;          /* current Px state */
+    uint8_t usable;       /* OUT: usable Px states (PMSTAT_get_pxstat) */
+    uint8_t last;         /* OUT: last Px state (PMSTAT_get_pxstat) */
+    uint8_t cur;          /* OUT: current Px state (PMSTAT_get_pxstat) */
     /*
      * OUT: Px transition table. This should have total * total elements.
      *      As it is a 2-D array, this will not be copied if it is smaller than
@@ -235,14 +235,33 @@ struct pm_px_stat {
 };
 
 struct pm_cx_stat {
-    uint32_t nr;    /* entry nr in triggers & residencies, including C0 */
-    uint32_t last;  /* last Cx state */
-    uint64_aligned_t idle_time;                 /* idle time from boot */
-    XEN_GUEST_HANDLE_64(uint64) triggers;    /* Cx trigger counts */
-    XEN_GUEST_HANDLE_64(uint64) residencies; /* Cx residencies */
-    uint32_t nr_pc;                          /* entry nr in pc[] */
-    uint32_t nr_cc;                          /* entry nr in cc[] */
     /*
+     * IN:  Number of elements in triggers, residencies (PMSTAT_get_cxstat)
+     * OUT: entry nr in triggers & residencies, including C0
+     *      (PMSTAT_get_cxstat, PMSTAT_get_max_cx)
+     */
+    uint32_t nr;
+    uint32_t last;  /* OUT: last Cx state (PMSTAT_get_cxstat) */
+    /* OUT: idle time from boot (PMSTAT_get_cxstat)*/
+    uint64_aligned_t idle_time;
+    /* OUT: Cx trigger counts, nr elements (PMSTAT_get_cxstat) */
+    XEN_GUEST_HANDLE_64(uint64) triggers;
+    /* OUT: Cx residencies, nr elements (PMSTAT_get_cxstat) */
+    XEN_GUEST_HANDLE_64(uint64) residencies;
+    /*
+     * IN: entry nr in pc[] (PMSTAT_get_cxstat)
+     * OUT: Required size of pc[] for all known to Xen entries to be written
+     *      (PMSTAT_get_cxstat)
+     */
+    uint32_t nr_pc;
+    /*
+     * IN: entry nr in cc[] (PMSTAT_get_cxstat)
+     * OUT: Required size of cc[] for all known to Xen entries to be written
+     *      (PMSTAT_get_cxstat)
+     */
+    uint32_t nr_cc;
+    /*
+     * OUT: (PMSTAT_get_cxstat)
      * These two arrays may (and generally will) have unused slots; slots not
      * having a corresponding hardware register will not be written by the
      * hypervisor. It is therefore up to the caller to put a suitable sentinel
-- 
2.49.0

[PATCH v3 3/5] cpufreq: Avoid potential buffer overrun and leak

[Ross Lagerwall]

If set_px_pminfo is called a second time with a larger state_count than
the first call, calls to PMSTAT_get_pxstat will read beyond the end of
the pt and trans_pt buffers allocated in cpufreq_statistic_init() since
they would have been allocated with the original state_count.

Secondly, the states array leaks on each subsequent call of
set_px_pminfo.

Fix both these issues by ignoring subsequent calls to set_px_pminfo if
it completed successfully previously. Return success rather than an
error to avoid errors in the dom0 kernel log when reloading the
xen_acpi_processor module.

At the same time, fix a leak of the states array on error.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---

In v3:

* Return success rather than an error when called a second time
* Use XFREE
 xen/drivers/cpufreq/cpufreq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/xen/drivers/cpufreq/cpufreq.c b/xen/drivers/cpufreq/cpufreq.c
index 19e29923356a..635f6e8c61a5 100644
--- a/xen/drivers/cpufreq/cpufreq.c
+++ b/xen/drivers/cpufreq/cpufreq.c
@@ -517,7 +517,7 @@ int set_px_pminfo(uint32_t acpi_id, struct xen_processor_performance *perf)
         }
     }
 
-    if ( perf->flags & XEN_PX_PSS )
+    if ( perf->flags & XEN_PX_PSS && !pxpt->states )
     {
         /* capability check */
         if ( perf->state_count <= 1 )
@@ -534,6 +534,7 @@ int set_px_pminfo(uint32_t acpi_id, struct xen_processor_performance *perf)
         }
         if ( copy_from_guest(pxpt->states, perf->states, perf->state_count) )
         {
+            XFREE(pxpt->states);
             ret = -EFAULT;
             goto out;
         }
-- 
2.49.0

[PATCH v3 4/5] libxc/PM: Ensure pxstat buffers are correctly sized

[Ross Lagerwall]

xc_pm_get_pxstat() requires the caller to allocate the pt and trans_pt
buffers but then calls xc_pm_get_max_px() to determine how big they are
(and hence how much Xen will copy into them). This is susceptible to
races if xc_pm_get_max_px() changes so avoid the problem by requiring
the caller to also pass in the size of the buffers.

Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---

In v3:

* Fix DECLARE_NAMED_HYPERCALL_BOUNCE size

 tools/libs/ctrl/xc_pm.c | 21 ++++++++++-----------
 tools/misc/xenpm.c      |  1 +
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/tools/libs/ctrl/xc_pm.c b/tools/libs/ctrl/xc_pm.c
index ff7b5ada053f..0bd79031044f 100644
--- a/tools/libs/ctrl/xc_pm.c
+++ b/tools/libs/ctrl/xc_pm.c
@@ -46,35 +46,34 @@ int xc_pm_get_pxstat(xc_interface *xch, int cpuid, struct xc_px_stat *pxpt)
 {
     struct xen_sysctl sysctl = {};
     /* Sizes unknown until xc_pm_get_max_px */
-    DECLARE_NAMED_HYPERCALL_BOUNCE(trans, pxpt->trans_pt, 0, XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
-    DECLARE_NAMED_HYPERCALL_BOUNCE(pt, pxpt->pt, 0, XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
+    DECLARE_NAMED_HYPERCALL_BOUNCE(trans, pxpt->trans_pt,
+                                   pxpt->total * pxpt->total * sizeof(uint64_t),
+                                   XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
+    DECLARE_NAMED_HYPERCALL_BOUNCE(pt, pxpt->pt,
+                                   pxpt->total * sizeof(struct xc_px_val),
+                                   XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
 
-    int max_px, ret;
+    int ret;
 
     if ( !pxpt->trans_pt || !pxpt->pt )
     {
         errno = EINVAL;
         return -1;
     }
-    if ( (ret = xc_pm_get_max_px(xch, cpuid, &max_px)) != 0)
-        return ret;
-
-    HYPERCALL_BOUNCE_SET_SIZE(trans, max_px * max_px * sizeof(uint64_t));
-    HYPERCALL_BOUNCE_SET_SIZE(pt, max_px * sizeof(struct xc_px_val));
 
     if ( xc_hypercall_bounce_pre(xch, trans) )
-        return ret;
+        return -1;
 
     if ( xc_hypercall_bounce_pre(xch, pt) )
     {
         xc_hypercall_bounce_post(xch, trans);
-        return ret;
+        return -1;
     }
 
     sysctl.cmd = XEN_SYSCTL_get_pmstat;
     sysctl.u.get_pmstat.type = PMSTAT_get_pxstat;
     sysctl.u.get_pmstat.cpuid = cpuid;
-    sysctl.u.get_pmstat.u.getpx.total = max_px;
+    sysctl.u.get_pmstat.u.getpx.total = pxpt->total;
     set_xen_guest_handle(sysctl.u.get_pmstat.u.getpx.trans_pt, trans);
     set_xen_guest_handle(sysctl.u.get_pmstat.u.getpx.pt, pt);
 
diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c
index db658ebaddd5..de319329e6b0 100644
--- a/tools/misc/xenpm.c
+++ b/tools/misc/xenpm.c
@@ -319,6 +319,7 @@ static int get_pxstat_by_cpuid(xc_interface *xc_handle, int cpuid, struct xc_px_
     if ( !pxstat)
         return -EINVAL;
 
+    pxstat->total = max_px_num;
     pxstat->trans_pt = malloc(max_px_num * max_px_num *
                               sizeof(uint64_t));
     if ( !pxstat->trans_pt )
-- 
2.49.0

[PATCH v3 5/5] libxc/PM: Retry get_pxstat if data is incomplete

[Ross Lagerwall]

If the total returned by Xen is more than the number of elements
allocated, it means that the buffer was too small and so the data is
incomplete. Retry to get all the data.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>
---
 tools/misc/xenpm.c | 49 +++++++++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 18 deletions(-)

diff --git a/tools/misc/xenpm.c b/tools/misc/xenpm.c
index de319329e6b0..d5387f5f0693 100644
--- a/tools/misc/xenpm.c
+++ b/tools/misc/xenpm.c
@@ -312,29 +312,42 @@ static int get_pxstat_by_cpuid(xc_interface *xc_handle, int cpuid, struct xc_px_
     int ret = 0;
     int max_px_num = 0;
 
-    ret = xc_pm_get_max_px(xc_handle, cpuid, &max_px_num);
-    if ( ret )
-        return -errno;
-
     if ( !pxstat)
         return -EINVAL;
 
-    pxstat->total = max_px_num;
-    pxstat->trans_pt = malloc(max_px_num * max_px_num *
-                              sizeof(uint64_t));
-    if ( !pxstat->trans_pt )
-        return -ENOMEM;
-    pxstat->pt = malloc(max_px_num * sizeof(struct xc_px_val));
-    if ( !pxstat->pt )
+    for ( ; ; )
     {
-        free(pxstat->trans_pt);
-        return -ENOMEM;
-    }
+        ret = xc_pm_get_max_px(xc_handle, cpuid, &max_px_num);
+        if ( ret )
+            return -errno;
 
-    ret = xc_pm_get_pxstat(xc_handle, cpuid, pxstat);
-    if( ret )
-    {
-        ret = -errno;
+        pxstat->total = max_px_num;
+        pxstat->trans_pt = malloc(max_px_num * max_px_num *
+                                  sizeof(uint64_t));
+        if ( !pxstat->trans_pt )
+            return -ENOMEM;
+        pxstat->pt = malloc(max_px_num * sizeof(struct xc_px_val));
+        if ( !pxstat->pt )
+        {
+            free(pxstat->trans_pt);
+            return -ENOMEM;
+        }
+
+        ret = xc_pm_get_pxstat(xc_handle, cpuid, pxstat);
+        if ( ret )
+        {
+            ret = -errno;
+            free(pxstat->trans_pt);
+            free(pxstat->pt);
+            pxstat->trans_pt = NULL;
+            pxstat->pt = NULL;
+            break;
+        }
+
+        if ( pxstat->total <= max_px_num )
+            break;
+
+        /* get_max_px changed under our feet so the data is incomplete. */
         free(pxstat->trans_pt);
         free(pxstat->pt);
         pxstat->trans_pt = NULL;
-- 
2.49.0

Re: [PATCH v3 4/5] libxc/PM: Ensure pxstat buffers are correctly sized

[Anthony PERARD]

On Tue, May 27, 2025 at 04:26:34PM +0100, Ross Lagerwall wrote:
> diff --git a/tools/libs/ctrl/xc_pm.c b/tools/libs/ctrl/xc_pm.c
> index ff7b5ada053f..0bd79031044f 100644
> --- a/tools/libs/ctrl/xc_pm.c
> +++ b/tools/libs/ctrl/xc_pm.c
> @@ -46,35 +46,34 @@ int xc_pm_get_pxstat(xc_interface *xch, int cpuid, struct xc_px_stat *pxpt)
>  {
>      struct xen_sysctl sysctl = {};
>      /* Sizes unknown until xc_pm_get_max_px */

This comment is wrong now and can be removed.

> -    DECLARE_NAMED_HYPERCALL_BOUNCE(trans, pxpt->trans_pt, 0, XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
> -    DECLARE_NAMED_HYPERCALL_BOUNCE(pt, pxpt->pt, 0, XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
> +    DECLARE_NAMED_HYPERCALL_BOUNCE(trans, pxpt->trans_pt,
> +                                   pxpt->total * pxpt->total * sizeof(uint64_t),
> +                                   XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
> +    DECLARE_NAMED_HYPERCALL_BOUNCE(pt, pxpt->pt,
> +                                   pxpt->total * sizeof(struct xc_px_val),
> +                                   XC_HYPERCALL_BUFFER_BOUNCE_BOTH);

The rest of the patch looks fine:
Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>

Thanks,

-- 
Anthony PERARD

Re: [PATCH v3 1/5] x86/pmstat: Check size of PMSTAT_get_pxstat buffers

[Jan Beulich]

On 27.05.2025 17:26, Ross Lagerwall wrote:
> --- a/xen/include/public/sysctl.h
> +++ b/xen/include/public/sysctl.h
> @@ -215,11 +215,22 @@ typedef struct pm_px_val pm_px_val_t;
>  DEFINE_XEN_GUEST_HANDLE(pm_px_val_t);
>  
>  struct pm_px_stat {
> -    uint8_t total;        /* total Px states */
> +    /*
> +     * IN: Number of elements in pt, number of rows/columns in trans_pt
> +     *     (PMSTAT_get_pxstat)
> +     * OUT: total Px states (PMSTAT_get_max_px, PMSTAT_get_pxstat)
> +     */
> +    uint8_t total;
>      uint8_t usable;       /* usable Px states */
>      uint8_t last;         /* last Px state */
>      uint8_t cur;          /* current Px state */
> -    XEN_GUEST_HANDLE_64(uint64) trans_pt;   /* Px transition table */
> +    /*
> +     * OUT: Px transition table. This should have total * total elements.
> +     *      As it is a 2-D array, this will not be copied if it is smaller than
> +     *      the hypervisor's Px transition table. (PMSTAT_get_pxstat)
> +     */
> +    XEN_GUEST_HANDLE_64(uint64) trans_pt;
> +    /* OUT: This should have total elements (PMSTAT_get_pxstat) */
>      XEN_GUEST_HANDLE_64(pm_px_val_t) pt;
>  };

Commentary here is still confusing imo: Since "total" now has two meanings,
saying "This should have .." in OUT: descriptions is ambiguous. Imo for
trans_pt you want to say something like "will not be copied if input total is
less than output total", and for pt "the number of elements copied is the
smaller of input and output total".

If that's okay with you, I can edit things along these lines while committing,
at which point
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

Re: [PATCH v3 2/5] public/sysctl: Clarify usage of pm_{px,cx}_stat

[Jan Beulich]

On 27.05.2025 17:26, Ross Lagerwall wrote:
> Suggested-by: Jan Beulich <jbeulich@suse.com>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v3 3/5] cpufreq: Avoid potential buffer overrun and leak

[Jan Beulich]

On 27.05.2025 17:26, Ross Lagerwall wrote:
> If set_px_pminfo is called a second time with a larger state_count than
> the first call, calls to PMSTAT_get_pxstat will read beyond the end of
> the pt and trans_pt buffers allocated in cpufreq_statistic_init() since
> they would have been allocated with the original state_count.
> 
> Secondly, the states array leaks on each subsequent call of
> set_px_pminfo.
> 
> Fix both these issues by ignoring subsequent calls to set_px_pminfo if
> it completed successfully previously. Return success rather than an
> error to avoid errors in the dom0 kernel log when reloading the
> xen_acpi_processor module.
> 
> At the same time, fix a leak of the states array on error.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH v3 1/5] x86/pmstat: Check size of PMSTAT_get_pxstat buffers

[Ross Lagerwall]

On Thu, Jun 5, 2025 at 11:10 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 27.05.2025 17:26, Ross Lagerwall wrote:
> > --- a/xen/include/public/sysctl.h
> > +++ b/xen/include/public/sysctl.h
> > @@ -215,11 +215,22 @@ typedef struct pm_px_val pm_px_val_t;
> >  DEFINE_XEN_GUEST_HANDLE(pm_px_val_t);
> >
> >  struct pm_px_stat {
> > -    uint8_t total;        /* total Px states */
> > +    /*
> > +     * IN: Number of elements in pt, number of rows/columns in trans_pt
> > +     *     (PMSTAT_get_pxstat)
> > +     * OUT: total Px states (PMSTAT_get_max_px, PMSTAT_get_pxstat)
> > +     */
> > +    uint8_t total;
> >      uint8_t usable;       /* usable Px states */
> >      uint8_t last;         /* last Px state */
> >      uint8_t cur;          /* current Px state */
> > -    XEN_GUEST_HANDLE_64(uint64) trans_pt;   /* Px transition table */
> > +    /*
> > +     * OUT: Px transition table. This should have total * total elements.
> > +     *      As it is a 2-D array, this will not be copied if it is smaller than
> > +     *      the hypervisor's Px transition table. (PMSTAT_get_pxstat)
> > +     */
> > +    XEN_GUEST_HANDLE_64(uint64) trans_pt;
> > +    /* OUT: This should have total elements (PMSTAT_get_pxstat) */
> >      XEN_GUEST_HANDLE_64(pm_px_val_t) pt;
> >  };
>
> Commentary here is still confusing imo: Since "total" now has two meanings,
> saying "This should have .." in OUT: descriptions is ambiguous. Imo for
> trans_pt you want to say something like "will not be copied if input total is
> less than output total", and for pt "the number of elements copied is the
> smaller of input and output total".
>
> If that's okay with you, I can edit things along these lines while committing,
> at which point
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>

Sure, that's fine with me.

Thanks,
Ross