From: Alan Modra Date: Sat, 30 Nov 2024 06:11:14 +0000 (+1030) Subject: Re: PR32399, buffer overflow printing core_file_failing_command X-Git-Tag: gdb-16-branchpoint~261 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=8ab91a033555c5faae1bcd615800670b91673731;p=thirdparty%2Fbinutils-gdb.git Re: PR32399, buffer overflow printing core_file_failing_command Fix more potential buffer overflows, and correct trad-code.c and cisco-core.c where they should be using bfd_{z}alloc rather than bfd_{z}malloc. To stop buffer overflows with fuzzed objects that don't have a terminator on the core_file_failing_command string, this patch allocates an extra byte at the end of the entire header buffer rather than poking a NUL at the end of the name array (u_comm[] or similar) because (a) it's better to not overwrite the file data, and (b) it is possible that some core files make use of fields in struct user beyond the end of u_comm to extend the command name. The patch also changes some unnecessary uses of bfd_zalloc to bfd_alloc. There's not much point in clearing memeory that will shortly be completely overwritten. PR 32399 * aix5ppc-core.c (xcoff64_core_p): Allocate an extra byte to ensure the core_file_failing_command string is terminated. * netbsd-core.c (netbsd_core_file_p): Likewise. * ptrace-core.c (ptrace_unix_core_file_p): Likewise. * rs6000-core.c (rs6000coff_core_p): Likewise. * trad-core.c (trad_unix_core_file_p): Likewise, and bfd_alloc tdata rather than bfd_zmalloc. * cisco-core.c (cisco_core_file_validate): bfd_zalloc tdata. --- diff --git a/bfd/aix5ppc-core.c b/bfd/aix5ppc-core.c index 179a7bf5b78..a6d6449fc57 100644 --- a/bfd/aix5ppc-core.c +++ b/bfd/aix5ppc-core.c @@ -66,8 +66,7 @@ xcoff64_core_p (bfd *abfd) if (bfd_seek (abfd, 0, SEEK_SET) != 0) goto xcoff64_core_p_error; - if (sizeof (struct core_dumpxx) - != bfd_read (&core, sizeof (struct core_dumpxx), abfd)) + if (sizeof core != bfd_read (&core, sizeof core, abfd)) goto xcoff64_core_p_error; if (bfd_stat (abfd, &statbuf) < 0) @@ -111,14 +110,16 @@ xcoff64_core_p (bfd *abfd) return NULL; } - new_core_hdr = bfd_zalloc (abfd, sizeof (struct core_dumpxx)); + new_core_hdr = bfd_alloc (abfd, sizeof (*new_core_hdr) + 1); if (NULL == new_core_hdr) return NULL; - memcpy (new_core_hdr, &core, sizeof (struct core_dumpxx)); - /* The core_hdr() macro is no longer used here because it would - expand to code relying on gcc's cast-as-lvalue extension, - which was removed in gcc 4.0. */ + memcpy (new_core_hdr, &core, sizeof (*new_core_hdr)); + + /* Ensure core_file_failing_command string is terminated. This is + just to stop buffer overflows on fuzzed files. */ + ((char *) new_core_hdr)[sizeof (*new_core_hdr)] = 0; + abfd->tdata.any = new_core_hdr; /* .stack section. */ diff --git a/bfd/cisco-core.c b/bfd/cisco-core.c index 75b11150f6d..1bbb44192ff 100644 --- a/bfd/cisco-core.c +++ b/bfd/cisco-core.c @@ -154,7 +154,7 @@ cisco_core_file_validate (bfd *abfd, int crash_info_loc) /* OK, we believe you. You're a core file. */ amt = sizeof (struct cisco_core_struct); - abfd->tdata.cisco_core_data = (struct cisco_core_struct *) bfd_zmalloc (amt); + abfd->tdata.cisco_core_data = bfd_zalloc (abfd, amt); if (abfd->tdata.cisco_core_data == NULL) return NULL; diff --git a/bfd/netbsd-core.c b/bfd/netbsd-core.c index 647af9d7bc2..ae56f3913e4 100644 --- a/bfd/netbsd-core.c +++ b/bfd/netbsd-core.c @@ -47,7 +47,7 @@ struct netbsd_core_struct { struct core core; -} *rawptr; +}; /* Handle NetBSD-style core dump file. */ @@ -60,9 +60,9 @@ netbsd_core_file_p (bfd *abfd) asection *asect; struct core core; struct coreseg coreseg; - size_t amt = sizeof core; + struct netbsd_core_struct *rawptr; - val = bfd_read (&core, amt, abfd); + val = bfd_read (&core, sizeof core, abfd); if (val != sizeof core) { /* Too small to be a core file. */ @@ -76,13 +76,15 @@ netbsd_core_file_p (bfd *abfd) return 0; } - amt = sizeof (struct netbsd_core_struct); - rawptr = (struct netbsd_core_struct *) bfd_zalloc (abfd, amt); + rawptr = bfd_alloc (abfd, sizeof (*rawptr) + 1); if (rawptr == NULL) return 0; - rawptr->core = core; abfd->tdata.netbsd_core_data = rawptr; + rawptr->core = core; + /* Ensure core_file_failing_command string is terminated. This is + just to stop buffer overflows on fuzzed files. */ + ((char *) rawptr)[sizeof (*rawptr)] = 0; offset = core.c_hdrsize; for (i = 0; i < core.c_nseg; i++) diff --git a/bfd/ptrace-core.c b/bfd/ptrace-core.c index 426d6070dc8..5952c06f8b6 100644 --- a/bfd/ptrace-core.c +++ b/bfd/ptrace-core.c @@ -61,7 +61,6 @@ ptrace_unix_core_file_p (bfd *abfd) int val; struct ptrace_user u; struct trad_core_struct *rawptr; - size_t amt; flagword flags; val = bfd_read (&u, sizeof u, abfd); @@ -77,8 +76,7 @@ ptrace_unix_core_file_p (bfd *abfd) /* Allocate both the upage and the struct core_data at once, so a single free() will free them both. */ - amt = sizeof (struct trad_core_struct); - rawptr = (struct trad_core_struct *) bfd_zalloc (abfd, amt); + rawptr = bfd_alloc (abfd, sizeof (*rawptr) + 1); if (rawptr == NULL) return 0; @@ -87,6 +85,10 @@ ptrace_unix_core_file_p (bfd *abfd) rawptr->u = u; /*Copy the uarea into the tdata part of the bfd */ + /* Ensure core_file_failing_command string is terminated. This is + just to stop buffer overflows on fuzzed files. */ + ((char *) rawptr)[sizeof (*rawptr)] = 0; + /* Create the sections. */ flags = SEC_ALLOC + SEC_LOAD + SEC_HAS_CONTENTS; diff --git a/bfd/rs6000-core.c b/bfd/rs6000-core.c index 19b9f46631f..ac8b29838ad 100644 --- a/bfd/rs6000-core.c +++ b/bfd/rs6000-core.c @@ -476,12 +476,15 @@ rs6000coff_core_p (bfd *abfd) #else size = sizeof (core.new_dump); #endif - tmpptr = (char *) bfd_zalloc (abfd, (bfd_size_type) size); + tmpptr = bfd_alloc (abfd, size + 1); if (!tmpptr) return NULL; /* Copy core file header. */ memcpy (tmpptr, &core, size); + /* Ensure core_file_failing_command string is terminated. This is + just to stop buffer overflows on fuzzed files. */ + tmpptr[size] = 0; set_tdata (abfd, tmpptr); /* Set architecture. */ diff --git a/bfd/trad-core.c b/bfd/trad-core.c index 012bc4bdd01..06b6bdadd87 100644 --- a/bfd/trad-core.c +++ b/bfd/trad-core.c @@ -65,7 +65,6 @@ trad_unix_core_file_p (bfd *abfd) int val; struct user u; struct trad_core_struct *rawptr; - size_t amt; flagword flags; #ifdef TRAD_CORE_USER_OFFSET @@ -132,8 +131,7 @@ trad_unix_core_file_p (bfd *abfd) /* Allocate both the upage and the struct core_data at once, so a single free() will free them both. */ - amt = sizeof (struct trad_core_struct); - rawptr = (struct trad_core_struct *) bfd_zmalloc (amt); + rawptr = bfd_alloc (abfd, sizeof (*rawptr) + 1); if (rawptr == NULL) return 0; @@ -141,6 +139,10 @@ trad_unix_core_file_p (bfd *abfd) rawptr->u = u; /*Copy the uarea into the tdata part of the bfd */ + /* Ensure core_file_failing_command string is terminated. This is + just to stop buffer overflows on fuzzed files. */ + ((char *) rawptr)[sizeof (*rawptr)] = 0; + /* Create the sections. */ flags = SEC_ALLOC + SEC_LOAD + SEC_HAS_CONTENTS;