| Message ID | 20250401130840.72119-4-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/EFI: prevent write-execute sections | expand |
On 01.04.2025 15:08, Roger Pau Monne wrote: > --- a/xen/arch/x86/efi/mkreloc.c > +++ b/xen/arch/x86/efi/mkreloc.c > @@ -17,6 +17,12 @@ > #define PE_BASE_RELOC_HIGHLOW 3 > #define PE_BASE_RELOC_DIR64 10 > > +/* The size of a symbol table entry is always 18 bytes. */ > +#define SYM_SIZE 18 > + > +const char *string_table; > +unsigned int string_table_size; The way you use it, imo this wants to be uint32_t. Both probably want to be static. > @@ -25,6 +31,28 @@ static void usage(const char *cmd, int rc) > exit(rc); > } > > +const char *get_name(const char *name) > +{ > + static char buffer[sizeof(((struct section_header *)NULL)->name) + 1] = {}; As this makes things section specific, may I suggest to name the function e.g. get_section_name()? Also better to be static again. > + unsigned long offset; > + > + if ( name[0] != '/' ) > + { > + /* > + * Use a temporary buffer in case the name is 8 characters long, as > + * then there's no terminating null character in the input string. > + */ > + strncpy(buffer, name, sizeof(buffer) - 1); > + return buffer; > + } > + > + offset = strtoul(&name[1], NULL, 10); Don't you need to nul-terminate the string here, too, to play safe? (Yes, we don't expect this big a string table.) > + if ( !string_table || offset < 4 || offset >= string_table_size ) Considering how you reduce string_table_size after having read it from the image, don't you mean "offset - 4 >= string_table_size" here? Also below you use sizeof(string_table_size) instead of a literal 4. > + return name; > + > + return &string_table[offset - 4]; Here as well. > @@ -83,6 +111,31 @@ static unsigned int load(const char *name, int *handle, > exit(3); > } > > + if ( !string_table && pe_hdr.symbol_table ) > + { > + char *strings; > + > + if ( lseek(in, pe_hdr.symbol_table + pe_hdr.symbols * SYM_SIZE, > + SEEK_SET) < 0 || > + read(in, &string_table_size, sizeof(string_table_size)) != > + sizeof(string_table_size) ) > + { > + perror(name); > + exit(3); > + } > + > + string_table_size -= sizeof(string_table_size); You're careful of underflow above; better be careful here, too? > + strings = malloc(string_table_size); You check for all other errors, just not here? Jan
On 01.04.2025 15:08, Roger Pau Monne wrote: > @@ -83,6 +111,31 @@ static unsigned int load(const char *name, int *handle, > exit(3); > } > > + if ( !string_table && pe_hdr.symbol_table ) > + { > + char *strings; > + > + if ( lseek(in, pe_hdr.symbol_table + pe_hdr.symbols * SYM_SIZE, > + SEEK_SET) < 0 || > + read(in, &string_table_size, sizeof(string_table_size)) != > + sizeof(string_table_size) ) > + { > + perror(name); > + exit(3); > + } > + > + string_table_size -= sizeof(string_table_size); > + strings = malloc(string_table_size); One more thing: Perhaps better to allocate an extra byte here, ... > + if ( read(in, strings, string_table_size) != string_table_size ) > + { > + perror(name); > + exit(3); > + } > + > + string_table = strings; > + } ... and then put a nul terminator at the end, just in case. Jan
diff --git a/xen/arch/x86/efi/mkreloc.c b/xen/arch/x86/efi/mkreloc.c index 1a6cfc845cba..cc106bd875ba 100644 --- a/xen/arch/x86/efi/mkreloc.c +++ b/xen/arch/x86/efi/mkreloc.c @@ -17,6 +17,12 @@ #define PE_BASE_RELOC_HIGHLOW 3 #define PE_BASE_RELOC_DIR64 10 +/* The size of a symbol table entry is always 18 bytes. */ +#define SYM_SIZE 18 + +const char *string_table; +unsigned int string_table_size; + static void usage(const char *cmd, int rc) { fprintf(rc ? stderr : stdout, @@ -25,6 +31,28 @@ static void usage(const char *cmd, int rc) exit(rc); } +const char *get_name(const char *name) +{ + static char buffer[sizeof(((struct section_header *)NULL)->name) + 1] = {}; + unsigned long offset; + + if ( name[0] != '/' ) + { + /* + * Use a temporary buffer in case the name is 8 characters long, as + * then there's no terminating null character in the input string. + */ + strncpy(buffer, name, sizeof(buffer) - 1); + return buffer; + } + + offset = strtoul(&name[1], NULL, 10); + if ( !string_table || offset < 4 || offset >= string_table_size ) + return name; + + return &string_table[offset - 4]; +} + static unsigned int load(const char *name, int *handle, struct section_header **sections, uint_fast64_t *image_base, @@ -83,6 +111,31 @@ static unsigned int load(const char *name, int *handle, exit(3); } + if ( !string_table && pe_hdr.symbol_table ) + { + char *strings; + + if ( lseek(in, pe_hdr.symbol_table + pe_hdr.symbols * SYM_SIZE, + SEEK_SET) < 0 || + read(in, &string_table_size, sizeof(string_table_size)) != + sizeof(string_table_size) ) + { + perror(name); + exit(3); + } + + string_table_size -= sizeof(string_table_size); + strings = malloc(string_table_size); + + if ( read(in, strings, string_table_size) != string_table_size ) + { + perror(name); + exit(3); + } + + string_table = strings; + } + *sections = malloc(pe_hdr.sections * sizeof(**sections)); if ( !*sections ) { @@ -173,8 +226,8 @@ static void diff_sections(const unsigned char *ptr1, const unsigned char *ptr2, if ( i < disp || i + width - disp > sec->raw_data_size ) { fprintf(stderr, - "Bogus difference at %.8s:%08" PRIxFAST32 "\n", - sec->name, i); + "Bogus difference at %s:%08" PRIxFAST32 "\n", + get_name(sec->name), i); exit(3); } @@ -184,9 +237,9 @@ static void diff_sections(const unsigned char *ptr1, const unsigned char *ptr2, if ( delta != diff ) { fprintf(stderr, - "Difference at %.8s:%08" PRIxFAST32 " is %#" PRIxFAST64 + "Difference at %s:%08" PRIxFAST32 " is %#" PRIxFAST64 " (expected %#" PRIxFAST64 ")\n", - sec->name, i - disp, delta, diff); + get_name(sec->name), i - disp, delta, diff); continue; } if ( width == 8 && (val1.u64 < base || val1.u64 > end) ) @@ -210,15 +263,15 @@ static void diff_sections(const unsigned char *ptr1, const unsigned char *ptr2, else if ( rva != cur_rva ) { fprintf(stderr, - "Cannot handle decreasing RVA (at %.8s:%08" PRIxFAST32 ")\n", - sec->name, i - disp); + "Cannot handle decreasing RVA (at %s:%08" PRIxFAST32 ")\n", + get_name(sec->name), i - disp); exit(3); } if ( !(sec->flags & IMAGE_SCN_MEM_WRITE) ) fprintf(stderr, - "Warning: relocation to r/o section %.8s:%08" PRIxFAST32 "\n", - sec->name, i - disp); + "Warning: relocation to r/o section %s:%08" PRIxFAST32 "\n", + get_name(sec->name), i - disp); printf("\t.word (%u << 12) | 0x%03" PRIxFAST32 "\n", reloc, sec->rva + i - disp - rva);
When using PE format names greater than 8 characters are placed in the string table, and a reference using the '/<offset>' format is placed in the name field. Read the string table if present, and decode names as required. No functional change intended, but the name references printed in error messages are now human readable: Warning: relocation to r/o section /4:00042d43 Becomes: Warning: relocation to r/o section .init.text:000446c3 Note the introduced helper to print names relies on a static internal buffer to make sure the returned string are always null terminated. This is enough for the current use-case, but if the returned value is to stay valid between calls the current static buffer won't work as expected. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/efi/mkreloc.c | 69 +++++++++++++++++++++++++++++++++----- 1 file changed, 61 insertions(+), 8 deletions(-)