| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 4 Jun 2018 15:14:04 -0700
From: Paul Burton <paul.burton@...s.com>
To: Thomas Bogendoerfer <tbogendoerfer@...e.de>
CC: Ralf Baechle <ralf@...ux-mips.org>,
James Hogan <jhogan@...nel.org>, <linux-mips@...ux-mips.org>,
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] Make elf2ecoff work on 64bit host machines
Hi Thomas,
On Mon, Jun 04, 2018 at 10:18:24AM +0200, Thomas Bogendoerfer wrote:
> Use fixed width integer types for ecoff structs to make elf2ecoff work
> on 64bit host machines
>
> Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@...e.de>
> ---
>
> v2: include stdint.h and use inttypes.h for printf formats
>
> arch/mips/boot/ecoff.h | 58 +++++++++++++++++++++++-----------------------
> arch/mips/boot/elf2ecoff.c | 31 +++++++++++++------------
> 2 files changed, 45 insertions(+), 44 deletions(-)
>
> diff --git a/arch/mips/boot/ecoff.h b/arch/mips/boot/ecoff.h
> index b3e73c22c345..9eb4167ef979 100644
> --- a/arch/mips/boot/ecoff.h
> +++ b/arch/mips/boot/ecoff.h
> @@ -3,13 +3,13 @@
> * Some ECOFF definitions.
> */
> typedef struct filehdr {
> - unsigned short f_magic; /* magic number */
> - unsigned short f_nscns; /* number of sections */
> - long f_timdat; /* time & date stamp */
> - long f_symptr; /* file pointer to symbolic header */
> - long f_nsyms; /* sizeof(symbolic hdr) */
> - unsigned short f_opthdr; /* sizeof(optional hdr) */
> - unsigned short f_flags; /* flags */
> + uint16_t f_magic; /* magic number */
> + uint16_t f_nscns; /* number of sections */
> + int32_t f_timdat; /* time & date stamp */
> + int32_t f_symptr; /* file pointer to symbolic header */
> + int32_t f_nsyms; /* sizeof(symbolic hdr) */
> + uint16_t f_opthdr; /* sizeof(optional hdr) */
> + uint16_t f_flags; /* flags */
> } FILHDR;
> #define FILHSZ sizeof(FILHDR)
>
> @@ -18,32 +18,32 @@ typedef struct filehdr {
>
> typedef struct scnhdr {
> char s_name[8]; /* section name */
> - long s_paddr; /* physical address, aliased s_nlib */
> - long s_vaddr; /* virtual address */
> - long s_size; /* section size */
> - long s_scnptr; /* file ptr to raw data for section */
> - long s_relptr; /* file ptr to relocation */
> - long s_lnnoptr; /* file ptr to gp histogram */
> - unsigned short s_nreloc; /* number of relocation entries */
> - unsigned short s_nlnno; /* number of gp histogram entries */
> - long s_flags; /* flags */
> + int32_t s_paddr; /* physical address, aliased s_nlib */
> + int32_t s_vaddr; /* virtual address */
> + int32_t s_size; /* section size */
> + int32_t s_scnptr; /* file ptr to raw data for section */
> + int32_t s_relptr; /* file ptr to relocation */
> + int32_t s_lnnoptr; /* file ptr to gp histogram */
> + uint16_t s_nreloc; /* number of relocation entries */
> + uint16_t s_nlnno; /* number of gp histogram entries */
> + int32_t s_flags; /* flags */
> } SCNHDR;
> #define SCNHSZ sizeof(SCNHDR)
> -#define SCNROUND ((long)16)
> +#define SCNROUND ((int32_t)16)
>
> typedef struct aouthdr {
> - short magic; /* see above */
> - short vstamp; /* version stamp */
> - long tsize; /* text size in bytes, padded to DW bdry*/
> - long dsize; /* initialized data " " */
> - long bsize; /* uninitialized data " " */
> - long entry; /* entry pt. */
> - long text_start; /* base of text used for this file */
> - long data_start; /* base of data used for this file */
> - long bss_start; /* base of bss used for this file */
> - long gprmask; /* general purpose register mask */
> - long cprmask[4]; /* co-processor register masks */
> - long gp_value; /* the gp value used for this object */
> + int16_t magic; /* see above */
> + int16_t vstamp; /* version stamp */
> + int32_t tsize; /* text size in bytes, padded to DW bdry*/
> + int32_t dsize; /* initialized data " " */
> + int32_t bsize; /* uninitialized data " " */
> + int32_t entry; /* entry pt. */
> + int32_t text_start; /* base of text used for this file */
> + int32_t data_start; /* base of data used for this file */
> + int32_t bss_start; /* base of bss used for this file */
> + int32_t gprmask; /* general purpose register mask */
> + int32_t cprmask[4]; /* co-processor register masks */
> + int32_t gp_value; /* the gp value used for this object */
> } AOUTHDR;
> #define AOUTHSZ sizeof(AOUTHDR)
>
> diff --git a/arch/mips/boot/elf2ecoff.c b/arch/mips/boot/elf2ecoff.c
> index 266c8137e859..b66eb3129e15 100644
> --- a/arch/mips/boot/elf2ecoff.c
> +++ b/arch/mips/boot/elf2ecoff.c
> @@ -43,6 +43,8 @@
> #include <limits.h>
> #include <netinet/in.h>
> #include <stdlib.h>
> +#include <stdint.h>
Could you move the #include <stdint.h> into ecoff.h? Since ecoff.h
itself makes use of these types. I know the end result will be the same,
but if anything else were ever to include ecoff.h then having the right
includes there could make that easier.
And I know that's unlikely to happen given that new platforms are
unlikely to use this but still, good practice.
> +#include <inttypes.h>
>
> #include "ecoff.h"
>
> @@ -55,8 +57,8 @@
> /* -------------------------------------------------------------------- */
>
> struct sect {
> - unsigned long vaddr;
> - unsigned long len;
> + uint32_t vaddr;
> + uint32_t len;
> };
>
> int *symTypeTable;
> @@ -153,16 +155,16 @@ static char *saveRead(int file, off_t offset, off_t len, char *name)
> }
>
> #define swab16(x) \
> - ((unsigned short)( \
> - (((unsigned short)(x) & (unsigned short)0x00ffU) << 8) | \
> - (((unsigned short)(x) & (unsigned short)0xff00U) >> 8) ))
> + ((uint16_t)( \
> + (((uint16_t)(x) & (uint16_t)0x00ffU) << 8) | \
> + (((uint16_t)(x) & (uint16_t)0xff00U) >> 8) ))
>
> #define swab32(x) \
> ((unsigned int)( \
> - (((unsigned int)(x) & (unsigned int)0x000000ffUL) << 24) | \
> - (((unsigned int)(x) & (unsigned int)0x0000ff00UL) << 8) | \
> - (((unsigned int)(x) & (unsigned int)0x00ff0000UL) >> 8) | \
> - (((unsigned int)(x) & (unsigned int)0xff000000UL) >> 24) ))
> + (((uint32_t)(x) & (uint32_t)0x000000ffUL) << 24) | \
> + (((uint32_t)(x) & (uint32_t)0x0000ff00UL) << 8) | \
> + (((uint32_t)(x) & (uint32_t)0x00ff0000UL) >> 8) | \
> + (((uint32_t)(x) & (uint32_t)0xff000000UL) >> 24) ))
>
> static void convert_elf_hdr(Elf32_Ehdr * e)
> {
> @@ -274,7 +276,7 @@ int main(int argc, char *argv[])
> struct aouthdr eah;
> struct scnhdr esecs[6];
> int infile, outfile;
> - unsigned long cur_vma = ULONG_MAX;
> + uint32_t cur_vma = UINT32_MAX;
> int addflag = 0;
> int nosecs;
>
> @@ -518,7 +520,7 @@ int main(int argc, char *argv[])
>
> for (i = 0; i < nosecs; i++) {
> printf
> - ("Section %d: %s phys %lx size %lx file offset %lx\n",
> + ("Section %d: %s phys %"PRIx32" size %"PRIx32" file offset %x\n",
The offset (s_scnptr) format should probably be PRIx32 as well.
I know you didn't introduce it but I think the tab before "file" in the
string would be better represented in source using \t rather than the
tab character itself, so perhaps you could change that for clarify here
which would also avoid making the line so long.
Apart from those niggles:
Reviewed-by: Paul Burton <paul.burton@...s.com>
Thanks,
Paul
> i, esecs[i].s_name, esecs[i].s_paddr,
> esecs[i].s_size, esecs[i].s_scnptr);
> }
> @@ -564,17 +566,16 @@ int main(int argc, char *argv[])
> the section can be loaded before copying. */
> if (ph[i].p_type == PT_LOAD && ph[i].p_filesz) {
> if (cur_vma != ph[i].p_vaddr) {
> - unsigned long gap =
> - ph[i].p_vaddr - cur_vma;
> + uint32_t gap = ph[i].p_vaddr - cur_vma;
> char obuf[1024];
> if (gap > 65536) {
> fprintf(stderr,
> - "Intersegment gap (%ld bytes) too large.\n",
> + "Intersegment gap (%"PRId32" bytes) too large.\n",
> gap);
> exit(1);
> }
> fprintf(stderr,
> - "Warning: %ld byte intersegment gap.\n",
> + "Warning: %d byte intersegment gap.\n",
> gap);
> memset(obuf, 0, sizeof obuf);
> while (gap) {
> --
> 2.13.6
>
>
Powered by blists - more mailing lists