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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, May 07, 2025 at 05:38:59PM +0100, Ross Lagerwall wrote:
> From: Kevin Lampis <kevin.lampis@xxxxxxxxx>
> 
> 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@xxxxxxxxx>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> ---
>  tools/libs/ctrl/xc_misc.c   |  3 +++
>  xen/common/livepatch.c      | 22 +++++++++++++++++-----
>  xen/include/public/sysctl.h | 12 ++++++++----
>  3 files changed, 28 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..72ef22bea5d2 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;
> +        uint32_t name_total_copied = 0, metadata_total_copied = 0;
>  
>          list_for_each_entry( data, &payload_list, list )
>          {
> @@ -1328,10 +1327,14 @@ 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 ( ((uint64_t)name_total_copied + name_len) > 
> list->name_total_size ||
> +                 ((uint64_t)metadata_total_copied + metadata_len) > 
> list->metadata_total_size )

I would define name_total_copied and metadata_total_copied as size_t,
and then avoid doing the cast to uint64_t here.

Also please adjust line length to 80 characters max.

Thanks, Roger.