[PATCH v2] livepatch: Pass buffer size to list sysctl

[PATCH v2] livepatch: Pass buffer size to list sysctl

[Ross Lagerwall]

From: Kevin Lampis <kevin.lampis@cloud.com>

The livepatch list sysctl writes metadata into a buffer provided by the
caller. The caller is expected to allocate an appropriately sized buffer
but this is racy and may result in Xen writing beyond the end of the
buffer should the metadata size change.

The name buffer is expected to be an array of elements with size
XEN_LIVEPATCH_NAME_SIZE to avoid this kind of race but the xen-livepatch
tool allocates only as many bytes as needed, therefore encountering the
same potential race condition.

Fix both these issues by requiring the caller to pass in the size of the
name and metadata buffers and then not writing beyond the allocated
size.

The sysctl interface version is bumped due to the change in semantics of
the fields.

Signed-off-by: Kevin Lampis <kevin.lampis@cloud.com>
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---

In v2:

Change type to size_t and fix line length.

 tools/libs/ctrl/xc_misc.c   |  3 +++
 xen/common/livepatch.c      | 23 ++++++++++++++++++-----
 xen/include/public/sysctl.h | 12 ++++++++----
 3 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/tools/libs/ctrl/xc_misc.c b/tools/libs/ctrl/xc_misc.c
index 6a60216bda03..33e87bac2868 100644
--- a/tools/libs/ctrl/xc_misc.c
+++ b/tools/libs/ctrl/xc_misc.c
@@ -867,6 +867,9 @@ int xc_livepatch_list(xc_interface *xch, const unsigned int max,
         set_xen_guest_handle(sysctl.u.livepatch.u.list.metadata, metadata);
         set_xen_guest_handle(sysctl.u.livepatch.u.list.metadata_len, metadata_len);
 
+        sysctl.u.livepatch.u.list.name_total_size = name_sz;
+        sysctl.u.livepatch.u.list.metadata_total_size = metadata_sz;
+
         rc = do_sysctl(xch, &sysctl);
         /*
          * From here on we MUST call xc_hypercall_bounce. If rc < 0 we
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index be9b7e367553..fc250c338da9 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -1311,11 +1311,10 @@ static int livepatch_list(struct xen_sysctl_livepatch_list *list)
         return -EINVAL;
     }
 
-    list->name_total_size = 0;
-    list->metadata_total_size = 0;
     if ( list->nr )
     {
         size_t name_offset = 0, metadata_offset = 0;
+        size_t name_total_copied = 0, metadata_total_copied = 0;
 
         list_for_each_entry( data, &payload_list, list )
         {
@@ -1328,10 +1327,15 @@ static int livepatch_list(struct xen_sysctl_livepatch_list *list)
             status.rc = data->rc;
 
             name_len = strlen(data->name) + 1;
-            list->name_total_size += name_len;
-
             metadata_len = data->metadata.len;
-            list->metadata_total_size += metadata_len;
+
+            if ( (name_total_copied + name_len) > list->name_total_size ||
+                 (metadata_total_copied + metadata_len) >
+                 list->metadata_total_size )
+            {
+                rc = -ENOMEM;
+                break;
+            }
 
             if ( !guest_handle_subrange_okay(list->name, name_offset,
                                              name_offset + name_len - 1) ||
@@ -1355,6 +1359,9 @@ static int livepatch_list(struct xen_sysctl_livepatch_list *list)
                 break;
             }
 
+            name_total_copied += name_len;
+            metadata_total_copied += metadata_len;
+
             idx++;
             name_offset += name_len;
             metadata_offset += metadata_len;
@@ -1362,9 +1369,15 @@ static int livepatch_list(struct xen_sysctl_livepatch_list *list)
             if ( (idx >= list->nr) || hypercall_preempt_check() )
                 break;
         }
+
+        list->name_total_size = name_total_copied;
+        list->metadata_total_size = metadata_total_copied;
     }
     else
     {
+        list->name_total_size = 0;
+        list->metadata_total_size = 0;
+
         list_for_each_entry( data, &payload_list, list )
         {
             list->name_total_size += strlen(data->name) + 1;
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index b0fec271d36f..9eca72865b87 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -26,9 +26,9 @@
  * (e.g. adding semantics to 0-checked input fields or data to zeroed output
  * fields) don't require a change of the version.
  *
- * Last version bump: Xen 4.17
+ * Last version bump: Xen 4.21
  */
-#define XEN_SYSCTL_INTERFACE_VERSION 0x00000015
+#define XEN_SYSCTL_INTERFACE_VERSION 0x00000016
 
 /*
  * Read console content from Xen buffer ring.
@@ -1101,8 +1101,12 @@ struct xen_sysctl_livepatch_list {
                                                amount of payloads and version.
                                                OUT: How many payloads left. */
     uint32_t pad;                           /* IN: Must be zero. */
-    uint32_t name_total_size;               /* OUT: Total size of all transfer names */
-    uint32_t metadata_total_size;           /* OUT: Total size of all transfer metadata */
+    uint32_t name_total_size;               /* IN: Size of name buffer
+                                               OUT: Total size of transferred
+                                               names */
+    uint32_t metadata_total_size;           /* IN: Size of metadata buffer
+                                               OUT: Total size of transferred
+                                               metadata */
     XEN_GUEST_HANDLE_64(xen_livepatch_status_t) status;  /* OUT. Must have enough
                                                space allocate for nr of them. */
     XEN_GUEST_HANDLE_64(char) name;         /* OUT: Array of names. Each member
-- 
2.49.0

Re: [PATCH v2] livepatch: Pass buffer size to list sysctl

[Roger Pau Monné]

On Thu, May 08, 2025 at 06:01:56PM +0100, Ross Lagerwall wrote:
> From: Kevin Lampis <kevin.lampis@cloud.com>
> 
> The livepatch list sysctl writes metadata into a buffer provided by the
> caller. The caller is expected to allocate an appropriately sized buffer
> but this is racy and may result in Xen writing beyond the end of the
> buffer should the metadata size change.
> 
> The name buffer is expected to be an array of elements with size
> XEN_LIVEPATCH_NAME_SIZE to avoid this kind of race but the xen-livepatch
> tool allocates only as many bytes as needed, therefore encountering the
> same potential race condition.
> 
> Fix both these issues by requiring the caller to pass in the size of the
> name and metadata buffers and then not writing beyond the allocated
> size.
> 
> The sysctl interface version is bumped due to the change in semantics of
> the fields.

I would be tempted to add:

Fixes: b145b4a39c13 ('livepatch: Handle arbitrary size names with the list operation')
Fixes: 5083e0ff939d ('livepatch: Add metadata runtime retrieval mechanism')

As the current approach can easily lead to buffer overruns in guest
memory, as Xen doesn't know the size.

> 
> Signed-off-by: Kevin Lampis <kevin.lampis@cloud.com>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.

Re: [PATCH v2] livepatch: Pass buffer size to list sysctl

[Jan Beulich]

On 08.05.2025 19:01, Ross Lagerwall wrote:
> @@ -1328,10 +1327,15 @@ static int livepatch_list(struct xen_sysctl_livepatch_list *list)
>              status.rc = data->rc;
>  
>              name_len = strlen(data->name) + 1;
> -            list->name_total_size += name_len;
> -
>              metadata_len = data->metadata.len;
> -            list->metadata_total_size += metadata_len;
> +
> +            if ( (name_total_copied + name_len) > list->name_total_size ||
> +                 (metadata_total_copied + metadata_len) >
> +                 list->metadata_total_size )
> +            {
> +                rc = -ENOMEM;

-ENOBUFS or maybe -ENOSPC, but certainly not -ENOMEM.

Jan

Re: [PATCH v2] livepatch: Pass buffer size to list sysctl

[Roger Pau Monné]

On Mon, May 12, 2025 at 11:51:35AM +0200, Jan Beulich wrote:
> On 08.05.2025 19:01, Ross Lagerwall wrote:
> > @@ -1328,10 +1327,15 @@ static int livepatch_list(struct xen_sysctl_livepatch_list *list)
> >              status.rc = data->rc;
> >  
> >              name_len = strlen(data->name) + 1;
> > -            list->name_total_size += name_len;
> > -
> >              metadata_len = data->metadata.len;
> > -            list->metadata_total_size += metadata_len;
> > +
> > +            if ( (name_total_copied + name_len) > list->name_total_size ||
> > +                 (metadata_total_copied + metadata_len) >
> > +                 list->metadata_total_size )
> > +            {
> > +                rc = -ENOMEM;
> 
> -ENOBUFS or maybe -ENOSPC, but certainly not -ENOMEM.

Jan, are you fine if I replace with -ENOBUFS on commit?

Thanks, Roger.

Re: [PATCH v2] livepatch: Pass buffer size to list sysctl

[Jan Beulich]

On 12.05.2025 12:54, Roger Pau Monné wrote:
> On Mon, May 12, 2025 at 11:51:35AM +0200, Jan Beulich wrote:
>> On 08.05.2025 19:01, Ross Lagerwall wrote:
>>> @@ -1328,10 +1327,15 @@ static int livepatch_list(struct xen_sysctl_livepatch_list *list)
>>>              status.rc = data->rc;
>>>  
>>>              name_len = strlen(data->name) + 1;
>>> -            list->name_total_size += name_len;
>>> -
>>>              metadata_len = data->metadata.len;
>>> -            list->metadata_total_size += metadata_len;
>>> +
>>> +            if ( (name_total_copied + name_len) > list->name_total_size ||
>>> +                 (metadata_total_copied + metadata_len) >
>>> +                 list->metadata_total_size )
>>> +            {
>>> +                rc = -ENOMEM;
>>
>> -ENOBUFS or maybe -ENOSPC, but certainly not -ENOMEM.
> 
> Jan, are you fine if I replace with -ENOBUFS on commit?

Yes.

Jan