]> git.ipfire.org Git - thirdparty/ipxe.git/commitdiff
[image] Make image data read-only to most consumers
authorMichael Brown <mcb30@ipxe.org>
Wed, 30 Apr 2025 13:14:51 +0000 (14:14 +0100)
committerMichael Brown <mcb30@ipxe.org>
Wed, 30 Apr 2025 14:38:15 +0000 (15:38 +0100)
Almost all image consumers do not need to modify the content of the
image.  Now that the image data is a pointer type (rather than the
opaque userptr_t type), we can rely on the compiler to enforce this at
build time.

Change the .data field to be a const pointer, so that the compiler can
verify that image consumers do not modify the image content.  Provide
a transparent .rwdata field for consumers who have a legitimate (and
now explicit) reason to modify the image content.

We do not attempt to impose any runtime restriction on checking
whether or not an image is writable.  The only existing instances of
genuinely read-only images are the various unit test images, and it is
acceptable for defective test cases to result in a segfault rather
than a runtime error.

Signed-off-by: Michael Brown <mcb30@ipxe.org>
15 files changed:
src/arch/x86/image/initrd.c
src/arch/x86/image/sdi.c
src/core/fdt.c
src/core/image.c
src/crypto/cms.c
src/image/efi_image.c
src/image/embedded.c
src/image/zlib.c
src/include/ipxe/image.h
src/interface/efi/efi_cmdline.c
src/tests/asn1_test.c
src/tests/asn1_test.h
src/tests/cms_test.c
src/tests/pixbuf_test.c
src/tests/pixbuf_test.h

index cb4879036fb0a0bd5386d19ef5bc58db6dc874b1..98c7a3804f0751a9c2aa45a81869d8a3daa78726 100644 (file)
@@ -63,10 +63,9 @@ static physaddr_t initrd_squash_high ( physaddr_t top ) {
                /* Find the highest image not yet in its final position */
                highest = NULL;
                for_each_image ( initrd ) {
-                       data = initrd->data;
-                       if ( ( virt_to_phys ( data ) < current ) &&
+                       if ( ( virt_to_phys ( initrd->data ) < current ) &&
                             ( ( highest == NULL ) ||
-                              ( virt_to_phys ( data ) >
+                              ( virt_to_phys ( initrd->data ) >
                                 virt_to_phys ( highest->data ) ) ) ) {
                                highest = initrd;
                        }
@@ -144,9 +143,9 @@ static void initrd_swap ( struct image *low, struct image *high,
 
                /* Swap fragments */
                memcpy ( free, ( high->data + len ), frag_len );
-               memmove ( ( low->data + new_len ), ( low->data + len ),
+               memmove ( ( low->rwdata + new_len ), ( low->data + len ),
                          low->len );
-               memcpy ( ( low->data + len ), free, frag_len );
+               memcpy ( ( low->rwdata + len ), free, frag_len );
                len = new_len;
        }
 
@@ -165,8 +164,8 @@ static void initrd_swap ( struct image *low, struct image *high,
 static int initrd_swap_any ( void *free, size_t free_len ) {
        struct image *low;
        struct image *high;
+       const void *adjacent;
        size_t padded_len;
-       void *adjacent;
 
        /* Find any pair of initrds that can be swapped */
        for_each_image ( low ) {
index cdfdeb3695def767cb0ffc61cacee008814c94fc..c0cded239f5635a854596380a11b49fcaea70237 100644 (file)
@@ -51,7 +51,7 @@ FEATURE ( FEATURE_IMAGE, "SDI", DHCP_EB_FEATURE_SDI, 1 );
  * @ret rc             Return status code
  */
 static int sdi_exec ( struct image *image ) {
-       struct sdi_header *sdi;
+       const struct sdi_header *sdi;
        uint32_t sdiptr;
 
        /* Sanity check */
index 7c7127aed86d25256a29a1d4acc3c58a6b4cd2ab..744e0e7054fc93decbd66efe63712b980c1febb9 100644 (file)
@@ -734,7 +734,7 @@ static int fdt_parse_image ( struct fdt *fdt, struct image *image ) {
        int rc;
 
        /* Parse image */
-       if ( ( rc = fdt_parse ( fdt, image->data, image->len ) ) != 0 ) {
+       if ( ( rc = fdt_parse ( fdt, image->rwdata, image->len ) ) != 0 ) {
                DBGC ( fdt, "FDT image \"%s\" is invalid: %s\n",
                       image->name, strerror ( rc ) );
                return rc;
index 4f92dfe57bef171cc661a2a0c741059213c2d122..a06466b7271bc071380d951515cb56ab75173fd4 100644 (file)
@@ -108,7 +108,7 @@ void free_image ( struct refcnt *refcnt ) {
 
        /* Free image data and image itself, if dynamically allocated */
        if ( ! ( image->flags & IMAGE_STATIC ) ) {
-               ufree ( image->data );
+               ufree ( image->rwdata );
                free ( image );
        }
 }
@@ -248,10 +248,10 @@ int image_set_len ( struct image *image, size_t len ) {
                return -ENOTTY;
 
        /* (Re)allocate image data */
-       new = urealloc ( image->data, len );
+       new = urealloc ( image->rwdata, len );
        if ( ! new )
                return -ENOMEM;
-       image->data = new;
+       image->rwdata = new;
        image->len = len;
 
        return 0;
@@ -273,7 +273,7 @@ int image_set_data ( struct image *image, const void *data, size_t len ) {
                return rc;
 
        /* Copy in new image data */
-       memcpy ( image->data, data, len );
+       memcpy ( image->rwdata, data, len );
 
        return 0;
 }
index 36b87c644a5bb096182e4e932f9c75276c29cd13..edfcc7fdcda410f27d1213d65b548b3dfd8a703a 100644 (file)
@@ -1079,7 +1079,7 @@ int cms_decrypt ( struct cms_message *cms, struct image *image,
        final_len = ( ( image->len && is_block_cipher ( cipher ) ) ?
                      cipher->blocksize : 0 );
        bulk_len = ( image->len - final_len );
-       cipher_decrypt ( cipher, ctx, image->data, image->data, bulk_len );
+       cipher_decrypt ( cipher, ctx, image->data, image->rwdata, bulk_len );
 
        /* Decrypt final block */
        cipher_decrypt ( cipher, ctx, ( image->data + bulk_len ), final,
@@ -1117,7 +1117,7 @@ int cms_decrypt ( struct cms_message *cms, struct image *image,
         * have to include include any error-handling code path to
         * reconstruct the block padding.
         */
-       memcpy ( ( image->data + bulk_len ), final, final_len );
+       memcpy ( ( image->rwdata + bulk_len ), final, final_len );
        image->len -= pad_len;
 
        /* Clear image type and re-register image, if applicable */
@@ -1137,7 +1137,7 @@ int cms_decrypt ( struct cms_message *cms, struct image *image,
         * containing the potentially invalid (and therefore
         * unreproducible) block padding.
         */
-       cipher_encrypt ( cipher, ctxdup, image->data, image->data, bulk_len );
+       cipher_encrypt ( cipher, ctxdup, image->data, image->rwdata, bulk_len );
        if ( original_flags & IMAGE_REGISTERED ) {
                register_image ( image ); /* Cannot fail on re-registration */
                image_put ( image );
index f7ee7ff50492a22ed01141e24adeded42fc64100..e7b19c4ce4ef5b292ac2f2885e5a366ad98506ef 100644 (file)
@@ -243,10 +243,15 @@ static int efi_image_exec ( struct image *image ) {
                goto err_shim_install;
        }
 
-       /* Attempt loading image */
+       /* Attempt loading image
+        *
+        * LoadImage() does not (allegedly) modify the image content,
+        * but requires a non-const pointer to SourceBuffer.  We
+        * therefore use the .rwdata field rather than .data.
+        */
        handle = NULL;
        if ( ( efirc = bs->LoadImage ( FALSE, efi_image_handle, path,
-                                      exec->data, exec->len,
+                                      exec->rwdata, exec->len,
                                       &handle ) ) != 0 ) {
                /* Not an EFI image */
                rc = -EEFI_LOAD ( efirc );
@@ -377,10 +382,15 @@ static int efi_image_probe ( struct image *image ) {
        EFI_STATUS efirc;
        int rc;
 
-       /* Attempt loading image */
+       /* Attempt loading image
+        *
+        * LoadImage() does not (allegedly) modify the image content,
+        * but requires a non-const pointer to SourceBuffer.  We
+        * therefore use the .rwdata field rather than .data.
+        */
        handle = NULL;
        if ( ( efirc = bs->LoadImage ( FALSE, efi_image_handle, &empty_path,
-                                      image->data, image->len,
+                                      image->rwdata, image->len,
                                       &handle ) ) != 0 ) {
                /* Not an EFI image */
                rc = -EEFI_LOAD ( efirc );
index 028f4930873df88d129d4473f21730fe26ea089e..76d256c9b55d186f6bb4e1af17fe1b0f7b30e3b6 100644 (file)
@@ -10,7 +10,6 @@ FILE_LICENCE ( GPL2_OR_LATER_OR_UBDL );
 
 #include <string.h>
 #include <ipxe/image.h>
-#include <ipxe/uaccess.h>
 #include <ipxe/init.h>
 
 /* Raw image data for all embedded images */
@@ -34,7 +33,7 @@ EMBED_ALL
        .refcnt = REF_INIT ( free_image ),                              \
        .name = _name,                                                  \
        .flags = ( IMAGE_STATIC | IMAGE_STATIC_NAME ),                  \
-       .data = ( userptr_t ) ( embedded_image_ ## _index ## _data ),   \
+       .rwdata = embedded_image_ ## _index ## _data,                   \
        .len = ( size_t ) embedded_image_ ## _index ## _len,            \
 },
 static struct image embedded_images[] = {
@@ -58,18 +57,8 @@ static void embedded_init ( void ) {
        for ( i = 0 ; i < ( int ) ( sizeof ( embedded_images ) /
                                    sizeof ( embedded_images[0] ) ) ; i++ ) {
                image = &embedded_images[i];
-
-               /* virt_to_user() cannot be used in a static
-                * initialiser, so we cast the pointer to a userptr_t
-                * in the initialiser and fix it up here.  (This will
-                * actually be a no-op on most platforms.)
-                */
-               data = ( ( void * ) image->data );
-               image->data = virt_to_user ( data );
-
                DBG ( "Embedded image \"%s\": %zd bytes at %p\n",
                      image->name, image->len, data );
-
                if ( ( rc = register_image ( image ) ) != 0 ) {
                        DBG ( "Could not register embedded image \"%s\": "
                              "%s\n", image->name, strerror ( rc ) );
index d7deee88b53d11231ffa5e722b32ae8f5eeddbb1..23eb50e7d7724462ab10f4c7191d431413896ed3 100644 (file)
@@ -65,7 +65,8 @@ int zlib_deflate ( enum deflate_format format, const void *data, size_t len,
                deflate_init ( deflate, format );
 
                /* Initialise output chunk */
-               deflate_chunk_init ( &out, extracted->data, 0, extracted->len );
+               deflate_chunk_init ( &out, extracted->rwdata, 0,
+                                    extracted->len );
 
                /* Decompress data */
                if ( ( rc = deflate_inflate ( deflate, data, len,
index 8ff938a17692ffb3fdbaea2ca90cb1b74296599b..fbf2b63b921c4137795fc8f5760fcf89c588b992 100644 (file)
@@ -46,7 +46,12 @@ struct image {
         * If the @c IMAGE_STATIC flag is set, then this is a
         * statically allocated image.
         */
-       void *data;
+       union {
+               /** Read-only data */
+               const void *data;
+               /** Writable data */
+               void *rwdata;
+       };
        /** Length of raw file image */
        size_t len;
 
index 59bce925f8362a9b6b4fc37fb6b7b592077baf20..d5ec6cee30b12f3c71bf9ba6f8bccf3070591150 100644 (file)
@@ -113,7 +113,7 @@ static int efi_cmdline_init ( void ) {
        DBGC ( colour, "CMDLINE using command line \"%s\"\n", cmdline );
 
        /* Prepare and register image */
-       efi_cmdline_image.data = virt_to_user ( cmdline );
+       efi_cmdline_image.data = cmdline;
        efi_cmdline_image.len = strlen ( cmdline );
        if ( efi_cmdline_image.len &&
             ( ( rc = register_image ( &efi_cmdline_image ) ) != 0 ) ) {
index b522b85d7b513fa69c42e11f4fdc6f25ca3c2a20..4760b97fb585074afd3382991a0bcc30dfa8a142 100644 (file)
@@ -59,9 +59,6 @@ void asn1_okx ( struct asn1_test *test, const char *file, unsigned int line ) {
        /* Sanity check */
        assert ( sizeof ( out ) == digest->digestsize );
 
-       /* Correct image data pointer */
-       test->image->data = virt_to_user ( ( void * ) test->image->data );
-
        /* Check that image is detected as correct type */
        okx ( register_image ( test->image ) == 0, file, line );
        okx ( test->image->type == test->type, file, line );
index f69a4bf2a8b81f6b8fca5bb2bb19dbe7d30e248c..f8310f5ba079943ddac40c1e153057a35398403c 100644 (file)
@@ -47,7 +47,7 @@ struct asn1_test {
                .refcnt = REF_INIT ( ref_no_free ),                     \
                .name = #_name,                                         \
                .flags = ( IMAGE_STATIC | IMAGE_STATIC_NAME ),          \
-               .data = ( userptr_t ) ( _name ## __file ),              \
+               .data = _name ## __file,                                \
                .len = sizeof ( _name ## __file ),                      \
        };                                                              \
        static struct asn1_test_digest _name ## _expected[] = {         \
index f80fbaa86f86e14cc2bae4a992579dbcff239eda..b71190cbabfce8d2abe1d475e6252add10b6fbcd 100644 (file)
@@ -37,7 +37,6 @@ FILE_LICENCE ( GPL2_OR_LATER_OR_UBDL );
 #include <ipxe/sha256.h>
 #include <ipxe/x509.h>
 #include <ipxe/image.h>
-#include <ipxe/uaccess.h>
 #include <ipxe/der.h>
 #include <ipxe/cms.h>
 #include <ipxe/privkey.h>
@@ -86,7 +85,7 @@ struct cms_test_keypair {
                        .refcnt = REF_INIT ( ref_no_free ),             \
                        .name = #NAME,                                  \
                        .flags = ( IMAGE_STATIC | IMAGE_STATIC_NAME ),  \
-                       .data = ( userptr_t ) ( NAME ## _data ),        \
+                       .data = NAME ## _data,                          \
                        .len = sizeof ( NAME ## _data ),                \
                },                                                      \
        }
@@ -99,7 +98,7 @@ struct cms_test_keypair {
                        .refcnt = REF_INIT ( ref_no_free ),             \
                        .name = #NAME,                                  \
                        .flags = ( IMAGE_STATIC | IMAGE_STATIC_NAME ),  \
-                       .data = ( userptr_t ) ( NAME ## _data ),        \
+                       .data = NAME ## _data,                          \
                        .len = sizeof ( NAME ## _data ),                \
                },                                                      \
        }
@@ -113,7 +112,7 @@ struct cms_test_keypair {
                        .name = #NAME,                                  \
                        .flags = ( IMAGE_STATIC | IMAGE_STATIC_NAME ),  \
                        .type = &der_image_type,                        \
-                       .data = ( userptr_t ) ( NAME ## _data ),        \
+                       .data = NAME ## _data,                          \
                        .len = sizeof ( NAME ## _data ),                \
                },                                                      \
        }
@@ -1652,16 +1651,9 @@ static time_t test_expired = 1375573111ULL; /* Sat Aug  3 23:38:31 2013 */
  */
 static void cms_message_okx ( struct cms_test_message *msg,
                              const char *file, unsigned int line ) {
-       const void *data = ( ( void * ) msg->image.data );
-
-       /* Fix up image data pointer */
-       msg->image.data = virt_to_user ( data );
 
        /* Check ability to parse message */
        okx ( cms_message ( &msg->image, &msg->cms ) == 0, file, line );
-
-       /* Reset image data pointer */
-       msg->image.data = ( ( userptr_t ) data );
 }
 #define cms_message_ok( msg ) \
        cms_message_okx ( msg, __FILE__, __LINE__ )
@@ -1705,10 +1697,6 @@ static void cms_verify_okx ( struct cms_test_message *msg,
                             time_t time, struct x509_chain *store,
                             struct x509_root *root, const char *file,
                             unsigned int line ) {
-       const void *data = ( ( void * ) img->image.data );
-
-       /* Fix up image data pointer */
-       img->image.data = virt_to_user ( data );
 
        /* Invalidate any certificates from previous tests */
        x509_invalidate_chain ( msg->cms->certificates );
@@ -1717,9 +1705,6 @@ static void cms_verify_okx ( struct cms_test_message *msg,
        okx ( cms_verify ( msg->cms, &img->image, name, time, store,
                           root ) == 0, file, line );
        okx ( img->image.flags & IMAGE_TRUSTED, file, line );
-
-       /* Reset image data pointer */
-       img->image.data = ( ( userptr_t ) data );
 }
 #define cms_verify_ok( msg, img, name, time, store, root )             \
        cms_verify_okx ( msg, img, name, time, store, root,             \
@@ -1742,10 +1727,6 @@ static void cms_verify_fail_okx ( struct cms_test_message *msg,
                                  time_t time, struct x509_chain *store,
                                  struct x509_root *root, const char *file,
                                  unsigned int line ) {
-       const void *data = ( ( void * ) img->image.data );
-
-       /* Fix up image data pointer */
-       img->image.data = virt_to_user ( data );
 
        /* Invalidate any certificates from previous tests */
        x509_invalidate_chain ( msg->cms->certificates );
@@ -1754,9 +1735,6 @@ static void cms_verify_fail_okx ( struct cms_test_message *msg,
        okx ( cms_verify ( msg->cms, &img->image, name, time, store,
                           root ) != 0, file, line );
        okx ( ! ( img->image.flags & IMAGE_TRUSTED ), file, line );
-
-       /* Reset image data pointer */
-       img->image.data = ( ( userptr_t ) data );
 }
 #define cms_verify_fail_ok( msg, img, name, time, store, root )        \
        cms_verify_fail_okx ( msg, img, name, time, store, root,        \
@@ -1777,10 +1755,6 @@ static void cms_decrypt_okx ( struct cms_test_image *img,
                              struct cms_test_keypair *keypair,
                              struct cms_test_image *expected,
                              const char *file, unsigned int line ) {
-       const void *data = ( ( void * ) img->image.data );
-
-       /* Fix up image data pointer */
-       img->image.data = virt_to_user ( data );
 
        /* Check ability to decrypt image */
        okx ( cms_decrypt ( envelope->cms, &img->image, NULL,
index a8ea1151eb0979bd148a1249755cbdd0cd57a104..cbc3a7617168c6ff047c8e7e874438590af64ab6 100644 (file)
@@ -55,9 +55,6 @@ void pixbuf_okx ( struct pixel_buffer_test *test, const char *file,
        assert ( ( test->width * test->height * sizeof ( test->data[0] ) )
                 == test->len );
 
-       /* Correct image data pointer */
-       test->image->data = virt_to_user ( ( void * ) test->image->data );
-
        /* Check that image is detected as correct type */
        okx ( register_image ( test->image ) == 0, file, line );
        okx ( test->image->type == test->type, file, line );
@@ -72,8 +69,8 @@ void pixbuf_okx ( struct pixel_buffer_test *test, const char *file,
 
                /* Check pixel buffer data */
                okx ( pixbuf->len == test->len, file, line );
-               okx ( memcmp ( pixbuf->data, virt_to_user ( test->data ),
-                              test->len ) == 0, file, line );
+               okx ( memcmp ( pixbuf->data, test->data, test->len ) == 0,
+                     file, line );
 
                pixbuf_put ( pixbuf );
        }
index 991c16ba31825873d0a18776615c5618d9d062a3..cf3d548f922d418d1cedad889836d47c3fbed6f0 100644 (file)
@@ -42,7 +42,7 @@ struct pixel_buffer_test {
                .refcnt = REF_INIT ( ref_no_free ),                     \
                .name = #_name,                                         \
                .flags = ( IMAGE_STATIC | IMAGE_STATIC_NAME ),          \
-               .data = ( userptr_t ) ( _name ## __file ),              \
+               .data = _name ## __file,                                \
                .len = sizeof ( _name ## __file ),                      \
        };                                                              \
        static struct pixel_buffer_test _name = {                       \