From: Michael Brown Date: Wed, 30 Apr 2025 12:22:54 +0000 (+0100) Subject: [image] Add the concept of a static image X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=cd803ff2e2424b56a7ae5886e4cfe17b47652e6e;p=thirdparty%2Fipxe.git [image] Add the concept of a static image Not all images are allocated via alloc_image(). For example: embedded images, the static images created to hold a runtime command line, and the images used by unit tests are all static structures. Using image_set_cmdline() (via e.g. the "imgargs" command) to set the command-line arguments of a static image will succeed but will leak memory, since nothing will ever free the allocated command line. There are no code paths that can lead to calling image_set_len() on a static image, but there is no safety check against future code paths attempting this. Define a flag IMAGE_STATIC to mark an image as statically allocated, generalise free_image() to also handle freeing dynamically allocated portions of static images (such as the command line), and expose free_image() for use by static images. Define a related flag IMAGE_STATIC_NAME to mark the name as statically allocated. Allow a statically allocated name to be replaced with a dynamically allocated name since this is a potentially valid use case (e.g. if "imgdecrypt --name " is used on an embedded image). Signed-off-by: Michael Brown --- diff --git a/src/arch/x86/core/runtime.c b/src/arch/x86/core/runtime.c index 461188d04..86083b1f9 100644 --- a/src/arch/x86/core/runtime.c +++ b/src/arch/x86/core/runtime.c @@ -70,6 +70,7 @@ static void cmdline_image_free ( struct refcnt *refcnt ) { struct image *image = container_of ( refcnt, struct image, refcnt ); DBGC ( image, "RUNTIME freeing command line\n" ); + free_image ( refcnt ); free ( cmdline_copy ); } @@ -77,6 +78,7 @@ static void cmdline_image_free ( struct refcnt *refcnt ) { static struct image cmdline_image = { .refcnt = REF_INIT ( cmdline_image_free ), .name = "", + .flags = ( IMAGE_STATIC | IMAGE_STATIC_NAME ), .type = &script_image_type, }; diff --git a/src/core/image.c b/src/core/image.c index 72885ec09..4f92dfe57 100644 --- a/src/core/image.c +++ b/src/core/image.c @@ -76,22 +76,41 @@ static int require_trusted_images_permanent = 0; * Free executable image * * @v refcnt Reference counter + * + * Image consumers must call image_put() rather than calling + * free_image() directly. This function is exposed for use only by + * static images. */ -static void free_image ( struct refcnt *refcnt ) { +void free_image ( struct refcnt *refcnt ) { struct image *image = container_of ( refcnt, struct image, refcnt ); struct image_tag *tag; + /* Sanity check: free_image() should not be called directly on + * dynamically allocated images. + */ + assert ( refcnt->count < 0 ); DBGC ( image, "IMAGE %s freed\n", image->name ); + + /* Clear any tag weak references */ for_each_table_entry ( tag, IMAGE_TAGS ) { if ( tag->image == image ) tag->image = NULL; } - free ( image->name ); + + /* Free dynamic allocations used by both static and dynamic images */ free ( image->cmdline ); uri_put ( image->uri ); - ufree ( image->data ); image_put ( image->replacement ); - free ( image ); + + /* Free image name, if dynamically allocated */ + if ( ! ( image->flags & IMAGE_STATIC_NAME ) ) + free ( image->name ); + + /* Free image data and image itself, if dynamically allocated */ + if ( ! ( image->flags & IMAGE_STATIC ) ) { + ufree ( image->data ); + free ( image ); + } } /** @@ -165,9 +184,13 @@ int image_set_name ( struct image *image, const char *name ) { if ( ! name_copy ) return -ENOMEM; + /* Free existing name, if not statically allocated */ + if ( ! ( image->flags & IMAGE_STATIC_NAME ) ) + free ( image->name ); + /* Replace existing name */ - free ( image->name ); image->name = name_copy; + image->flags &= ~IMAGE_STATIC_NAME; return 0; } @@ -220,6 +243,10 @@ int image_set_cmdline ( struct image *image, const char *cmdline ) { int image_set_len ( struct image *image, size_t len ) { void *new; + /* Refuse to reallocate static images */ + if ( image->flags & IMAGE_STATIC ) + return -ENOTTY; + /* (Re)allocate image data */ new = urealloc ( image->data, len ); if ( ! new ) @@ -288,6 +315,13 @@ int register_image ( struct image *image ) { char name[8]; /* "imgXXXX" */ int rc; + /* Sanity checks */ + if ( image->flags & IMAGE_STATIC ) { + assert ( ( image->name == NULL ) || + ( image->flags & IMAGE_STATIC_NAME ) ); + assert ( image->cmdline == NULL ); + } + /* Create image name if it doesn't already have one */ if ( ! image->name ) { snprintf ( name, sizeof ( name ), "img%d", imgindex++ ); diff --git a/src/image/embedded.c b/src/image/embedded.c index 58833ac30..028f49308 100644 --- a/src/image/embedded.c +++ b/src/image/embedded.c @@ -31,8 +31,9 @@ EMBED_ALL /* Image structures for all embedded images */ #undef EMBED #define EMBED( _index, _path, _name ) { \ - .refcnt = REF_INIT ( ref_no_free ), \ + .refcnt = REF_INIT ( free_image ), \ .name = _name, \ + .flags = ( IMAGE_STATIC | IMAGE_STATIC_NAME ), \ .data = ( userptr_t ) ( embedded_image_ ## _index ## _data ), \ .len = ( size_t ) embedded_image_ ## _index ## _len, \ }, diff --git a/src/include/ipxe/image.h b/src/include/ipxe/image.h index bf2d77626..8ff938a17 100644 --- a/src/include/ipxe/image.h +++ b/src/include/ipxe/image.h @@ -30,14 +30,22 @@ struct image { /** URI of image */ struct uri *uri; - /** Name */ + /** Name + * + * If the @c IMAGE_STATIC_NAME flag is set, then this is a + * statically allocated string. + */ char *name; /** Flags */ unsigned int flags; /** Command line to pass to image */ char *cmdline; - /** Raw file image */ + /** Raw file image + * + * If the @c IMAGE_STATIC flag is set, then this is a + * statically allocated image. + */ void *data; /** Length of raw file image */ size_t len; @@ -72,6 +80,12 @@ struct image { /** Image will be hidden from enumeration */ #define IMAGE_HIDDEN 0x0008 +/** Image is statically allocated */ +#define IMAGE_STATIC 0x0010 + +/** Image name is statically allocated */ +#define IMAGE_STATIC_NAME 0x0020 + /** An executable image type */ struct image_type { /** Name of this image type */ @@ -185,6 +199,7 @@ static inline struct image * first_image ( void ) { return list_first_entry ( &images, struct image, list ); } +extern void free_image ( struct refcnt *refcnt ); extern struct image * alloc_image ( struct uri *uri ); extern int image_set_uri ( struct image *image, struct uri *uri ); extern int image_set_name ( struct image *image, const char *name ); diff --git a/src/interface/efi/efi_cmdline.c b/src/interface/efi/efi_cmdline.c index 13ad0fc35..59bce925f 100644 --- a/src/interface/efi/efi_cmdline.c +++ b/src/interface/efi/efi_cmdline.c @@ -58,6 +58,7 @@ static void efi_cmdline_free ( struct refcnt *refcnt ) { struct image *image = container_of ( refcnt, struct image, refcnt ); DBGC ( image, "CMDLINE freeing command line\n" ); + free_image ( refcnt ); free ( efi_cmdline_copy ); } @@ -65,6 +66,7 @@ static void efi_cmdline_free ( struct refcnt *refcnt ) { static struct image efi_cmdline_image = { .refcnt = REF_INIT ( efi_cmdline_free ), .name = "", + .flags = ( IMAGE_STATIC | IMAGE_STATIC_NAME ), .type = &script_image_type, }; diff --git a/src/tests/asn1_test.h b/src/tests/asn1_test.h index c8167ed36..f69a4bf2a 100644 --- a/src/tests/asn1_test.h +++ b/src/tests/asn1_test.h @@ -46,6 +46,7 @@ struct asn1_test { static struct image _name ## __image = { \ .refcnt = REF_INIT ( ref_no_free ), \ .name = #_name, \ + .flags = ( IMAGE_STATIC | IMAGE_STATIC_NAME ), \ .data = ( userptr_t ) ( _name ## __file ), \ .len = sizeof ( _name ## __file ), \ }; \ diff --git a/src/tests/cms_test.c b/src/tests/cms_test.c index 5e186cdf0..f80fbaa86 100644 --- a/src/tests/cms_test.c +++ b/src/tests/cms_test.c @@ -85,6 +85,7 @@ struct cms_test_keypair { .image = { \ .refcnt = REF_INIT ( ref_no_free ), \ .name = #NAME, \ + .flags = ( IMAGE_STATIC | IMAGE_STATIC_NAME ), \ .data = ( userptr_t ) ( NAME ## _data ), \ .len = sizeof ( NAME ## _data ), \ }, \ @@ -97,6 +98,7 @@ struct cms_test_keypair { .image = { \ .refcnt = REF_INIT ( ref_no_free ), \ .name = #NAME, \ + .flags = ( IMAGE_STATIC | IMAGE_STATIC_NAME ), \ .data = ( userptr_t ) ( NAME ## _data ), \ .len = sizeof ( NAME ## _data ), \ }, \ @@ -109,6 +111,7 @@ struct cms_test_keypair { .image = { \ .refcnt = REF_INIT ( ref_no_free ), \ .name = #NAME, \ + .flags = ( IMAGE_STATIC | IMAGE_STATIC_NAME ), \ .type = &der_image_type, \ .data = ( userptr_t ) ( NAME ## _data ), \ .len = sizeof ( NAME ## _data ), \ diff --git a/src/tests/pixbuf_test.h b/src/tests/pixbuf_test.h index d12829d89..991c16ba3 100644 --- a/src/tests/pixbuf_test.h +++ b/src/tests/pixbuf_test.h @@ -41,6 +41,7 @@ struct pixel_buffer_test { static struct image _name ## __image = { \ .refcnt = REF_INIT ( ref_no_free ), \ .name = #_name, \ + .flags = ( IMAGE_STATIC | IMAGE_STATIC_NAME ), \ .data = ( userptr_t ) ( _name ## __file ), \ .len = sizeof ( _name ## __file ), \ }; \ diff --git a/src/tests/test.c b/src/tests/test.c index 1ec4b21ef..9fa12e27a 100644 --- a/src/tests/test.c +++ b/src/tests/test.c @@ -162,6 +162,7 @@ static struct image_type test_image_type = { static struct image test_image = { .refcnt = REF_INIT ( ref_no_free ), .name = "", + .flags = ( IMAGE_STATIC | IMAGE_STATIC_NAME ), .type = &test_image_type, };