[PATCH 0/4] Allows Secure Boot for Kexec

[PATCH 0/4] Allows Secure Boot for Kexec

[Frediano Ziglio]

Using EFI Secure Boot all kernel level code should be signed and
there should be no way to run unchecked code.
For this reason the Kexec interface needs to be changed in order
to allows signature checking.

The purgatory code is included in Xen itself as passing this code
from userspace it's not secure (see patches 2/4 and 3/4).

Ross Lagerwall (4):
  xen/lib: Export additional sha256 functions
  kexec: Include purgatory in Xen
  kexec: Implement new EFI load types
  kexec: Support non-page-aligned kexec segments

 xen/arch/arm/Makefile                 |   1 +
 xen/arch/arm/kexec.c                  |  27 +
 xen/arch/x86/Makefile                 |   2 +
 xen/arch/x86/bzimage.c                |  40 +-
 xen/arch/x86/kexec.c                  | 125 +++++
 xen/arch/x86/purgatory/.gitignore     |   3 +
 xen/arch/x86/purgatory/Makefile       |  64 +++
 xen/arch/x86/purgatory/config.h       |  37 ++
 xen/arch/x86/purgatory/entry64.S      | 108 ++++
 xen/arch/x86/purgatory/purgatory.c    |  59 +++
 xen/arch/x86/purgatory/setup-x86_64.S |  63 +++
 xen/arch/x86/purgatory/stack.S        |  21 +
 xen/common/Kconfig                    |   1 +
 xen/common/kexec.c                    |  33 +-
 xen/common/kimage.c                   | 703 ++++++++++++++++++++++++--
 xen/include/public/kexec.h            |  23 +-
 xen/include/xen/kimage.h              |  57 ++-
 xen/include/xen/sha2.h                |  10 +
 xen/include/xen/x86-linux.h           |  62 +++
 xen/lib/sha2-256.c                    |  16 +-
 20 files changed, 1344 insertions(+), 111 deletions(-)
 create mode 100644 xen/arch/arm/kexec.c
 create mode 100644 xen/arch/x86/kexec.c
 create mode 100644 xen/arch/x86/purgatory/.gitignore
 create mode 100644 xen/arch/x86/purgatory/Makefile
 create mode 100644 xen/arch/x86/purgatory/config.h
 create mode 100644 xen/arch/x86/purgatory/entry64.S
 create mode 100644 xen/arch/x86/purgatory/purgatory.c
 create mode 100644 xen/arch/x86/purgatory/setup-x86_64.S
 create mode 100644 xen/arch/x86/purgatory/stack.S
 create mode 100644 xen/include/xen/x86-linux.h

-- 
2.43.0

[PATCH 1/4] xen/lib: Export additional sha256 functions

[Frediano Ziglio]

From: Ross Lagerwall <ross.lagerwall@citrix.com>

In future, some code needs to generate a digest over several separate buffers
so export the necessary functions to do so.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/include/xen/sha2.h | 10 ++++++++++
 xen/lib/sha2-256.c     | 14 ++++----------
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/xen/include/xen/sha2.h b/xen/include/xen/sha2.h
index 47d97fbf01..ea8bad67e4 100644
--- a/xen/include/xen/sha2.h
+++ b/xen/include/xen/sha2.h
@@ -9,6 +9,16 @@
 
 #define SHA2_256_DIGEST_SIZE 32
 
+struct sha2_256_state {
+    uint32_t state[SHA2_256_DIGEST_SIZE / sizeof(uint32_t)];
+    uint8_t buf[64];
+    size_t count; /* Byte count. */
+};
+
+void sha2_256_init(struct sha2_256_state *s);
+void sha2_256_update(struct sha2_256_state *s, const void *msg,
+                     size_t len);
+void sha2_256_final(struct sha2_256_state *s, void *_dst);
 void sha2_256_digest(uint8_t digest[SHA2_256_DIGEST_SIZE],
                      const void *msg, size_t len);
 
diff --git a/xen/lib/sha2-256.c b/xen/lib/sha2-256.c
index 19e8252188..896a257ed9 100644
--- a/xen/lib/sha2-256.c
+++ b/xen/lib/sha2-256.c
@@ -10,12 +10,6 @@
 #include <xen/string.h>
 #include <xen/unaligned.h>
 
-struct sha2_256_state {
-    uint32_t state[SHA2_256_DIGEST_SIZE / sizeof(uint32_t)];
-    uint8_t buf[64];
-    size_t count; /* Byte count. */
-};
-
 static uint32_t choose(uint32_t x, uint32_t y, uint32_t z)
 {
     return z ^ (x & (y ^ z));
@@ -131,7 +125,7 @@ static void sha2_256_transform(uint32_t *state, const void *_input)
     state[4] += e; state[5] += f; state[6] += g; state[7] += h;
 }
 
-static void sha2_256_init(struct sha2_256_state *s)
+void sha2_256_init(struct sha2_256_state *s)
 {
     *s = (struct sha2_256_state){
         .state = {
@@ -147,8 +141,8 @@ static void sha2_256_init(struct sha2_256_state *s)
     };
 }
 
-static void sha2_256_update(struct sha2_256_state *s, const void *msg,
-                            size_t len)
+void sha2_256_update(struct sha2_256_state *s, const void *msg,
+                     size_t len)
 {
     unsigned int partial = s->count & 63;
 
@@ -177,7 +171,7 @@ static void sha2_256_update(struct sha2_256_state *s, const void *msg,
     memcpy(s->buf + partial, msg, len);
 }
 
-static void sha2_256_final(struct sha2_256_state *s, void *_dst)
+void sha2_256_final(struct sha2_256_state *s, void *_dst)
 {
     uint32_t *dst = _dst;
     unsigned int i, partial = s->count & 63;
-- 
2.43.0

[PATCH 4/4] kexec: Support non-page-aligned kexec segments

[Frediano Ziglio]

From: Ross Lagerwall <ross.lagerwall@citrix.com>

With Secure Boot, userspace passes in the entire kernel loaded for verification
purposes. However, the kernel's startup32 function needs to be aligned (e.g. to
16 MiB) and this results in the start of the segment not being page-aligned
(depending on where the startup32 function lands in the kernel binary). Relax
this restriction in Xen to support this use case.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/common/kexec.c       | 23 +++++++-----
 xen/common/kimage.c      | 81 +++++++++++++++++++++-------------------
 xen/include/xen/kimage.h | 15 +++++++-
 3 files changed, 70 insertions(+), 49 deletions(-)

diff --git a/xen/common/kexec.c b/xen/common/kexec.c
index 158f8da6fd..a7b3958c74 100644
--- a/xen/common/kexec.c
+++ b/xen/common/kexec.c
@@ -910,7 +910,7 @@ static uint16_t kexec_load_v1_arch(void)
 }
 
 static int kexec_segments_add_segment(unsigned int *nr_segments,
-                                      xen_kexec_segment_t *segments,
+                                      struct kimage_segment *segments,
                                       mfn_t mfn)
 {
     paddr_t maddr = mfn_to_maddr(mfn);
@@ -936,7 +936,7 @@ static int kexec_segments_add_segment(unsigned int *nr_segments,
 
 static int kexec_segments_from_ind_page(mfn_t mfn,
                                         unsigned int *nr_segments,
-                                        xen_kexec_segment_t *segments,
+                                        struct kimage_segment *segments,
                                         bool compat)
 {
     void *page;
@@ -991,7 +991,7 @@ done:
 static int kexec_do_load_v1(xen_kexec_load_v1_t *load, int compat)
 {
     struct kexec_image *kimage = NULL;
-    xen_kexec_segment_t *segments;
+    struct kimage_segment *segments;
     uint16_t arch;
     unsigned int nr_segments = 0;
     mfn_t ind_mfn = maddr_to_mfn(load->image.indirection_page);
@@ -1001,7 +1001,7 @@ static int kexec_do_load_v1(xen_kexec_load_v1_t *load, int compat)
     if ( arch == EM_NONE )
         return -ENOSYS;
 
-    segments = xmalloc_array(xen_kexec_segment_t, KEXEC_SEGMENT_MAX);
+    segments = xmalloc_array(struct kimage_segment, KEXEC_SEGMENT_MAX);
     if ( segments == NULL )
         return -ENOMEM;
 
@@ -1103,9 +1103,10 @@ static int kexec_load_v1_compat(XEN_GUEST_HANDLE_PARAM(void) uarg)
 static int kexec_load(XEN_GUEST_HANDLE_PARAM(void) uarg)
 {
     xen_kexec_load_t load;
-    xen_kexec_segment_t *segments;
+    struct kimage_segment *segments;
     struct kexec_image *kimage = NULL;
     int ret;
+    unsigned int i;
 
     if ( copy_from_guest(&load, uarg, 1) )
         return -EFAULT;
@@ -1113,14 +1114,18 @@ static int kexec_load(XEN_GUEST_HANDLE_PARAM(void) uarg)
     if ( load.nr_segments >= KEXEC_SEGMENT_MAX )
         return -EINVAL;
 
-    segments = xmalloc_array(xen_kexec_segment_t, load.nr_segments);
+    segments = xmalloc_array(struct kimage_segment, load.nr_segments);
     if ( segments == NULL )
         return -ENOMEM;
 
-    if ( copy_from_guest(segments, load.segments.h, load.nr_segments) )
+    for ( i = 0; i < load.nr_segments; i++ )
     {
-        ret = -EFAULT;
-        goto error;
+        if ( copy_from_guest_offset((xen_kexec_segment_t *)&segments[i],
+                                    load.segments.h, i, 1) )
+        {
+            ret = -EFAULT;
+            goto error;
+        }
     }
 
     ret = kimage_alloc(&kimage, load.type, load.arch, load.entry_maddr,
diff --git a/xen/common/kimage.c b/xen/common/kimage.c
index 212f5bd068..296febeb09 100644
--- a/xen/common/kimage.c
+++ b/xen/common/kimage.c
@@ -96,7 +96,7 @@ static struct page_info *kimage_alloc_zeroed_page(unsigned memflags)
 
 static int do_kimage_alloc(struct kexec_image **rimage, paddr_t entry,
                            unsigned long nr_segments,
-                           xen_kexec_segment_t *segments, uint8_t type)
+                           struct kimage_segment *segments, uint8_t type)
 {
     struct kexec_image *image;
     unsigned long i;
@@ -119,29 +119,6 @@ static int do_kimage_alloc(struct kexec_image **rimage, paddr_t entry,
     INIT_PAGE_LIST_HEAD(&image->dest_pages);
     INIT_PAGE_LIST_HEAD(&image->unusable_pages);
 
-    /*
-     * Verify we have good destination addresses.  The caller is
-     * responsible for making certain we don't attempt to load the new
-     * image into invalid or reserved areas of RAM.  This just
-     * verifies it is an address we can use.
-     *
-     * Since the kernel does everything in page size chunks ensure the
-     * destination addresses are page aligned.  Too many special cases
-     * crop of when we don't do this.  The most insidious is getting
-     * overlapping destination addresses simply because addresses are
-     * changed to page size granularity.
-     */
-    result = -EADDRNOTAVAIL;
-    for ( i = 0; i < nr_segments; i++ )
-    {
-        paddr_t mstart, mend;
-
-        mstart = image->segments[i].dest_maddr;
-        mend   = mstart + image->segments[i].dest_size;
-        if ( (mstart & ~PAGE_MASK) || (mend & ~PAGE_MASK) )
-            goto out;
-    }
-
     /*
      * Verify our destination addresses do not overlap.  If we allowed
      * overlapping destination addresses through very weird things can
@@ -221,7 +198,7 @@ out:
 
 static int kimage_normal_alloc(struct kexec_image **rimage, paddr_t entry,
                                unsigned long nr_segments,
-                               xen_kexec_segment_t *segments)
+                               struct kimage_segment *segments)
 {
     return do_kimage_alloc(rimage, entry, nr_segments, segments,
                            KEXEC_TYPE_DEFAULT);
@@ -229,7 +206,7 @@ static int kimage_normal_alloc(struct kexec_image **rimage, paddr_t entry,
 
 static int do_kimage_crash_alloc(struct kexec_image **rimage, paddr_t entry,
                                  unsigned long nr_segments,
-                                 xen_kexec_segment_t *segments)
+                                 struct kimage_segment *segments)
 {
     unsigned long i;
 
@@ -264,7 +241,7 @@ static int do_kimage_crash_alloc(struct kexec_image **rimage, paddr_t entry,
 
 static int kimage_crash_alloc(struct kexec_image **rimage, paddr_t entry,
                               unsigned long nr_segments,
-                              xen_kexec_segment_t *segments)
+                              struct kimage_segment *segments)
 {
     /* Verify we have a valid entry point */
     if ( (entry < kexec_crash_area.start)
@@ -276,7 +253,7 @@ static int kimage_crash_alloc(struct kexec_image **rimage, paddr_t entry,
 
 static int kimage_crash_alloc_efi(struct kexec_image **rimage, paddr_t entry,
                                   unsigned long nr_segments,
-                                  xen_kexec_segment_t *segments)
+                                  struct kimage_segment *segments)
 {
     return do_kimage_crash_alloc(rimage, entry, nr_segments, segments);
 }
@@ -694,16 +671,18 @@ found:
 }
 
 static int kimage_load_normal_segment(struct kexec_image *image,
-                                      xen_kexec_segment_t *segment)
+                                      struct kimage_segment *segment)
 {
     unsigned long to_copy;
     unsigned long src_offset;
+    unsigned int dest_offset;
     paddr_t dest, end;
     int ret;
 
     to_copy = segment->buf_size;
     src_offset = 0;
     dest = segment->dest_maddr;
+    dest_offset = segment->dest_offset;
 
     ret = kimage_set_destination(image, dest);
     if ( ret < 0 )
@@ -718,7 +697,7 @@ static int kimage_load_normal_segment(struct kexec_image *image,
 
         dest_mfn = dest >> PAGE_SHIFT;
 
-        size = min_t(unsigned long, PAGE_SIZE, to_copy);
+        size = min_t(unsigned long, PAGE_SIZE - dest_offset, to_copy);
 
         page = kimage_alloc_page(image, dest);
         if ( !page )
@@ -728,7 +707,7 @@ static int kimage_load_normal_segment(struct kexec_image *image,
             return ret;
 
         dest_va = __map_domain_page(page);
-        ret = copy_from_guest_offset(dest_va, segment->buf.h, src_offset, size);
+        ret = copy_from_guest_offset(dest_va + dest_offset, segment->buf.h, src_offset, size);
         unmap_domain_page(dest_va);
         if ( ret )
             return -EFAULT;
@@ -736,6 +715,7 @@ static int kimage_load_normal_segment(struct kexec_image *image,
         to_copy -= size;
         src_offset += size;
         dest += PAGE_SIZE;
+        dest_offset = 0;
     }
 
     /* Remainder of the destination should be zeroed. */
@@ -747,7 +727,7 @@ static int kimage_load_normal_segment(struct kexec_image *image,
 }
 
 static int kimage_load_crash_segment(struct kexec_image *image,
-                                     xen_kexec_segment_t *segment)
+                                     struct kimage_segment *segment)
 {
     /*
      * For crash dumps kernels we simply copy the data from user space
@@ -755,12 +735,14 @@ static int kimage_load_crash_segment(struct kexec_image *image,
      */
     paddr_t dest;
     unsigned long sbytes, dbytes;
+    unsigned int dest_offset;
     int ret = 0;
     unsigned long src_offset = 0;
 
     sbytes = segment->buf_size;
     dbytes = segment->dest_size;
     dest = segment->dest_maddr;
+    dest_offset = segment->dest_offset;
 
     while ( dbytes )
     {
@@ -770,14 +752,16 @@ static int kimage_load_crash_segment(struct kexec_image *image,
 
         dest_mfn = dest >> PAGE_SHIFT;
 
-        dchunk = PAGE_SIZE;
+        dchunk = PAGE_SIZE - dest_offset;
         schunk = min(dchunk, sbytes);
 
         dest_va = map_domain_page(_mfn(dest_mfn));
         if ( !dest_va )
             return -EINVAL;
 
-        ret = copy_from_guest_offset(dest_va, segment->buf.h,
+        if ( dest_offset )
+            memset(dest_va, 0, dest_offset);
+        ret = copy_from_guest_offset(dest_va + dest_offset, segment->buf.h,
                                      src_offset, schunk);
         memset(dest_va + schunk, 0, dchunk - schunk);
 
@@ -785,17 +769,18 @@ static int kimage_load_crash_segment(struct kexec_image *image,
         if ( ret )
             return -EFAULT;
 
-        dbytes -= dchunk;
+        dbytes -= dchunk + dest_offset;
         sbytes -= schunk;
-        dest += dchunk;
+        dest += dchunk + dest_offset;
         src_offset += schunk;
+        dest_offset = 0;
     }
 
     return 0;
 }
 
 static int kimage_load_segment(struct kexec_image *image,
-                               xen_kexec_segment_t *segment)
+                               struct kimage_segment *segment)
 {
     int result = -ENOMEM;
     paddr_t addr;
@@ -826,9 +811,29 @@ static int kimage_load_segment(struct kexec_image *image,
 
 int kimage_alloc(struct kexec_image **rimage, uint8_t type, uint16_t arch,
                  uint64_t entry_maddr,
-                 uint32_t nr_segments, xen_kexec_segment_t *segment)
+                 uint32_t nr_segments, struct kimage_segment *segment)
 {
     int result;
+    unsigned int i;
+
+    for ( i = 0; i < nr_segments; i++ )
+    {
+        paddr_t mend;
+
+        /*
+         * Stash the destination offset-in-page for use when copying the
+         * buffer later.
+         */
+        segment[i].dest_offset = PAGE_OFFSET(segment[i].dest_maddr);
+
+        /*
+         * Align down the start address to page size and align up the end
+         * address to page size.
+         */
+        mend = segment[i].dest_maddr + segment[i].dest_size;
+        segment[i].dest_maddr &= PAGE_MASK;
+        segment[i].dest_size = ROUNDUP(mend, PAGE_SIZE) - segment[i].dest_maddr;
+    }
 
     switch( type )
     {
diff --git a/xen/include/xen/kimage.h b/xen/include/xen/kimage.h
index 6626058f8b..3099b489b5 100644
--- a/xen/include/xen/kimage.h
+++ b/xen/include/xen/kimage.h
@@ -30,6 +30,17 @@ struct purgatory_info {
     Elf_Shdr *sechdrs;
 };
 
+struct kimage_segment {
+    union {
+        XEN_GUEST_HANDLE(const_void) h;
+        uint64_t _pad;
+    } buf;
+    uint64_t buf_size;
+    uint64_t dest_maddr;
+    uint64_t dest_size;
+    unsigned int dest_offset;
+};
+
 typedef struct xen_kexec_regs {
         uint64_t rax;
         uint64_t rbx;
@@ -55,7 +66,7 @@ struct kexec_image {
     uint16_t arch;
     uint64_t entry_maddr;
     uint32_t nr_segments;
-    xen_kexec_segment_t *segments;
+    struct kimage_segment *segments;
 
     kimage_entry_t head;
     struct page_info *entry_page;
@@ -77,7 +88,7 @@ struct kexec_image {
 
 int kimage_alloc(struct kexec_image **rimage, uint8_t type, uint16_t arch,
                  uint64_t entry_maddr,
-                 uint32_t nr_segments, xen_kexec_segment_t *segment);
+                 uint32_t nr_segments, struct kimage_segment *segment);
 void kimage_free(struct kexec_image *image);
 int kimage_load_segments(struct kexec_image *image);
 struct page_info *kimage_alloc_control_page(struct kexec_image *image,
-- 
2.43.0

Re: [PATCH 1/4] xen/lib: Export additional sha256 functions

[Andrew Cooper]

On 06/05/2025 2:56 pm, Frediano Ziglio wrote:
> diff --git a/xen/include/xen/sha2.h b/xen/include/xen/sha2.h
> index 47d97fbf01..ea8bad67e4 100644
> --- a/xen/include/xen/sha2.h
> +++ b/xen/include/xen/sha2.h
> @@ -9,6 +9,16 @@
>  
>  #define SHA2_256_DIGEST_SIZE 32
>  
> +struct sha2_256_state {
> +    uint32_t state[SHA2_256_DIGEST_SIZE / sizeof(uint32_t)];
> +    uint8_t buf[64];
> +    size_t count; /* Byte count. */
> +};
> +
> +void sha2_256_init(struct sha2_256_state *s);
> +void sha2_256_update(struct sha2_256_state *s, const void *msg,
> +                     size_t len);
> +void sha2_256_final(struct sha2_256_state *s, void *_dst);
>  void sha2_256_digest(uint8_t digest[SHA2_256_DIGEST_SIZE],
>                       const void *msg, size_t len);

sha2_256_digest() is unlike the others as it holds sha2_256_state
internally.  I'd suggest having all of the additions below this point,
which group them more nicely.

Can fix on commit.  Otherwise LGTM.

~Andrew

Re: [PATCH 0/4] Allows Secure Boot for Kexec

[Andrii Sultanov]

The 2nd and 3rd patches got lost somewhere and do not seem to be shown on lore,
at the very least.

Re: [PATCH 1/4] xen/lib: Export additional sha256 functions

[Andrew Cooper]

On 06/05/2025 3:05 pm, Andrew Cooper wrote:
> On 06/05/2025 2:56 pm, Frediano Ziglio wrote:
>> diff --git a/xen/include/xen/sha2.h b/xen/include/xen/sha2.h
>> index 47d97fbf01..ea8bad67e4 100644
>> --- a/xen/include/xen/sha2.h
>> +++ b/xen/include/xen/sha2.h
>> @@ -9,6 +9,16 @@
>>  
>>  #define SHA2_256_DIGEST_SIZE 32
>>  
>> +struct sha2_256_state {
>> +    uint32_t state[SHA2_256_DIGEST_SIZE / sizeof(uint32_t)];
>> +    uint8_t buf[64];
>> +    size_t count; /* Byte count. */
>> +};
>> +
>> +void sha2_256_init(struct sha2_256_state *s);
>> +void sha2_256_update(struct sha2_256_state *s, const void *msg,
>> +                     size_t len);
>> +void sha2_256_final(struct sha2_256_state *s, void *_dst);
>>  void sha2_256_digest(uint8_t digest[SHA2_256_DIGEST_SIZE],
>>                       const void *msg, size_t len);
> sha2_256_digest() is unlike the others as it holds sha2_256_state
> internally.  I'd suggest having all of the additions below this point,
> which group them more nicely.
>
> Can fix on commit.  Otherwise LGTM.

Not quite.  Now that sha2_256_final() is exported, it should have a
proper type for  _dst.  I've folded:

diff --git a/xen/include/xen/sha2.h b/xen/include/xen/sha2.h
index 0d55fe640431..cb30e8f8d77c 100644
--- a/xen/include/xen/sha2.h
+++ b/xen/include/xen/sha2.h
@@ -21,6 +21,7 @@ struct sha2_256_state {
 void sha2_256_init(struct sha2_256_state *s);
 void sha2_256_update(struct sha2_256_state *s, const void *msg,
                      size_t len);
-void sha2_256_final(struct sha2_256_state *s, void *_dst);
+void sha2_256_final(struct sha2_256_state *s,
+                    uint8_t digest[SHA2_256_DIGEST_SIZE]);
 
 #endif /* XEN_SHA2_H */
diff --git a/xen/lib/sha2-256.c b/xen/lib/sha2-256.c
index 896a257ed9b7..08ef7011a1c3 100644
--- a/xen/lib/sha2-256.c
+++ b/xen/lib/sha2-256.c
@@ -171,9 +171,9 @@ void sha2_256_update(struct sha2_256_state *s, const
void *msg,
     memcpy(s->buf + partial, msg, len);
 }
 
-void sha2_256_final(struct sha2_256_state *s, void *_dst)
+void sha2_256_final(struct sha2_256_state *s, uint8_t
digest[SHA2_256_DIGEST_SIZE])
 {
-    uint32_t *dst = _dst;
+    uint32_t *dst = (uint32_t *)digest;
     unsigned int i, partial = s->count & 63;
 
     /* Start padding */

in too, which is compatible with the rest of the series too.

~Andrew

Re: [PATCH 1/4] xen/lib: Export additional sha256 functions

[Frediano Ziglio]

On Tue, May 6, 2025 at 11:17 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 06/05/2025 3:05 pm, Andrew Cooper wrote:
> > On 06/05/2025 2:56 pm, Frediano Ziglio wrote:
> >> diff --git a/xen/include/xen/sha2.h b/xen/include/xen/sha2.h
> >> index 47d97fbf01..ea8bad67e4 100644
> >> --- a/xen/include/xen/sha2.h
> >> +++ b/xen/include/xen/sha2.h
> >> @@ -9,6 +9,16 @@
> >>
> >>  #define SHA2_256_DIGEST_SIZE 32
> >>
> >> +struct sha2_256_state {
> >> +    uint32_t state[SHA2_256_DIGEST_SIZE / sizeof(uint32_t)];
> >> +    uint8_t buf[64];
> >> +    size_t count; /* Byte count. */
> >> +};
> >> +
> >> +void sha2_256_init(struct sha2_256_state *s);
> >> +void sha2_256_update(struct sha2_256_state *s, const void *msg,
> >> +                     size_t len);
> >> +void sha2_256_final(struct sha2_256_state *s, void *_dst);
> >>  void sha2_256_digest(uint8_t digest[SHA2_256_DIGEST_SIZE],
> >>                       const void *msg, size_t len);
> > sha2_256_digest() is unlike the others as it holds sha2_256_state
> > internally.  I'd suggest having all of the additions below this point,
> > which group them more nicely.
> >
> > Can fix on commit.  Otherwise LGTM.
>
> Not quite.  Now that sha2_256_final() is exported, it should have a
> proper type for  _dst.  I've folded:
>
> diff --git a/xen/include/xen/sha2.h b/xen/include/xen/sha2.h
> index 0d55fe640431..cb30e8f8d77c 100644
> --- a/xen/include/xen/sha2.h
> +++ b/xen/include/xen/sha2.h
> @@ -21,6 +21,7 @@ struct sha2_256_state {
>  void sha2_256_init(struct sha2_256_state *s);
>  void sha2_256_update(struct sha2_256_state *s, const void *msg,
>                       size_t len);
> -void sha2_256_final(struct sha2_256_state *s, void *_dst);
> +void sha2_256_final(struct sha2_256_state *s,
> +                    uint8_t digest[SHA2_256_DIGEST_SIZE]);
>
>  #endif /* XEN_SHA2_H */
> diff --git a/xen/lib/sha2-256.c b/xen/lib/sha2-256.c
> index 896a257ed9b7..08ef7011a1c3 100644
> --- a/xen/lib/sha2-256.c
> +++ b/xen/lib/sha2-256.c
> @@ -171,9 +171,9 @@ void sha2_256_update(struct sha2_256_state *s, const
> void *msg,
>      memcpy(s->buf + partial, msg, len);
>  }
>
> -void sha2_256_final(struct sha2_256_state *s, void *_dst)
> +void sha2_256_final(struct sha2_256_state *s, uint8_t
> digest[SHA2_256_DIGEST_SIZE])
>  {
> -    uint32_t *dst = _dst;
> +    uint32_t *dst = (uint32_t *)digest;

That is we are never going to support architectures with unaligned
memory access.

>      unsigned int i, partial = s->count & 63;
>
>      /* Start padding */
>
> in too, which is compatible with the rest of the series too.
>
> ~Andrew

I'll send an updated series with these and all the commits (mail server issues).

Frediano

Re: [PATCH 1/4] xen/lib: Export additional sha256 functions

[Andrew Cooper]

On 07/05/2025 6:26 am, Frediano Ziglio wrote:
> On Tue, May 6, 2025 at 11:17 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 06/05/2025 3:05 pm, Andrew Cooper wrote:
>>> On 06/05/2025 2:56 pm, Frediano Ziglio wrote:
>>>> diff --git a/xen/include/xen/sha2.h b/xen/include/xen/sha2.h
>>>> index 47d97fbf01..ea8bad67e4 100644
>>>> --- a/xen/include/xen/sha2.h
>>>> +++ b/xen/include/xen/sha2.h
>>>> @@ -9,6 +9,16 @@
>>>>
>>>>  #define SHA2_256_DIGEST_SIZE 32
>>>>
>>>> +struct sha2_256_state {
>>>> +    uint32_t state[SHA2_256_DIGEST_SIZE / sizeof(uint32_t)];
>>>> +    uint8_t buf[64];
>>>> +    size_t count; /* Byte count. */
>>>> +};
>>>> +
>>>> +void sha2_256_init(struct sha2_256_state *s);
>>>> +void sha2_256_update(struct sha2_256_state *s, const void *msg,
>>>> +                     size_t len);
>>>> +void sha2_256_final(struct sha2_256_state *s, void *_dst);
>>>>  void sha2_256_digest(uint8_t digest[SHA2_256_DIGEST_SIZE],
>>>>                       const void *msg, size_t len);
>>> sha2_256_digest() is unlike the others as it holds sha2_256_state
>>> internally.  I'd suggest having all of the additions below this point,
>>> which group them more nicely.
>>>
>>> Can fix on commit.  Otherwise LGTM.
>> Not quite.  Now that sha2_256_final() is exported, it should have a
>> proper type for  _dst.  I've folded:
>>
>> diff --git a/xen/include/xen/sha2.h b/xen/include/xen/sha2.h
>> index 0d55fe640431..cb30e8f8d77c 100644
>> --- a/xen/include/xen/sha2.h
>> +++ b/xen/include/xen/sha2.h
>> @@ -21,6 +21,7 @@ struct sha2_256_state {
>>  void sha2_256_init(struct sha2_256_state *s);
>>  void sha2_256_update(struct sha2_256_state *s, const void *msg,
>>                       size_t len);
>> -void sha2_256_final(struct sha2_256_state *s, void *_dst);
>> +void sha2_256_final(struct sha2_256_state *s,
>> +                    uint8_t digest[SHA2_256_DIGEST_SIZE]);
>>
>>  #endif /* XEN_SHA2_H */
>> diff --git a/xen/lib/sha2-256.c b/xen/lib/sha2-256.c
>> index 896a257ed9b7..08ef7011a1c3 100644
>> --- a/xen/lib/sha2-256.c
>> +++ b/xen/lib/sha2-256.c
>> @@ -171,9 +171,9 @@ void sha2_256_update(struct sha2_256_state *s, const
>> void *msg,
>>      memcpy(s->buf + partial, msg, len);
>>  }
>>
>> -void sha2_256_final(struct sha2_256_state *s, void *_dst)
>> +void sha2_256_final(struct sha2_256_state *s, uint8_t
>> digest[SHA2_256_DIGEST_SIZE])
>>  {
>> -    uint32_t *dst = _dst;
>> +    uint32_t *dst = (uint32_t *)digest;
> That is we are never going to support architectures with unaligned
> memory access.

Note how it's written to.

    /* Store state in digest */
    for ( i = 0; i < 8; i++ )
        put_unaligned_be32(s->state[i], &dst[i]);

It works just fine on misaligned buffers.

~Andrew

Re: [PATCH 1/4] xen/lib: Export additional sha256 functions

[Jan Beulich]

On 06.05.2025 16:05, Andrew Cooper wrote:
> On 06/05/2025 2:56 pm, Frediano Ziglio wrote:
>> diff --git a/xen/include/xen/sha2.h b/xen/include/xen/sha2.h
>> index 47d97fbf01..ea8bad67e4 100644
>> --- a/xen/include/xen/sha2.h
>> +++ b/xen/include/xen/sha2.h
>> @@ -9,6 +9,16 @@
>>  
>>  #define SHA2_256_DIGEST_SIZE 32
>>  
>> +struct sha2_256_state {
>> +    uint32_t state[SHA2_256_DIGEST_SIZE / sizeof(uint32_t)];
>> +    uint8_t buf[64];
>> +    size_t count; /* Byte count. */
>> +};
>> +
>> +void sha2_256_init(struct sha2_256_state *s);
>> +void sha2_256_update(struct sha2_256_state *s, const void *msg,
>> +                     size_t len);
>> +void sha2_256_final(struct sha2_256_state *s, void *_dst);
>>  void sha2_256_digest(uint8_t digest[SHA2_256_DIGEST_SIZE],
>>                       const void *msg, size_t len);
> 
> sha2_256_digest() is unlike the others as it holds sha2_256_state
> internally.  I'd suggest having all of the additions below this point,
> which group them more nicely.
> 
> Can fix on commit.  Otherwise LGTM.

I notice this was committed, but isn't this introducing new Misra
violations (extern functions without external callers)?

Jan