This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH] gdb/riscv: Prevent buffer overflow in riscv_return_value
- From: Andrew Burgess <andrew dot burgess at embecosm dot com>
- To: gdb-patches at sourceware dot org
- Cc: jimw at sifive dot com, palmer at sifive dot com, jhb at FreeBSD dot org
- Date: Wed, 12 Dec 2018 15:19:35 +0000
- Subject: Re: [PATCH] gdb/riscv: Prevent buffer overflow in riscv_return_value
- References: <20181128220414.13367-1-andrew.burgess@embecosm.com>
I plan to push this change in the next few days unless I get some
objections.
Thanks,
Andrew
* Andrew Burgess <andrew.burgess@embecosm.com> [2018-11-28 22:04:14 +0000]:
> The existing code for reading and writing the return value can
> overflow the passed in buffers in a couple of situations. This commit
> aims to resolve these issues.
>
> The problems were detected using valgrind, here are two examples,
> first from gdb.base/structs.exp:
>
> (gdb) p/x fun9()
> ==31353== Invalid write of size 8
> ==31353== at 0x4C34153: memmove (vg_replace_strmem.c:1270)
> ==31353== by 0x632EBB: memcpy (string_fortified.h:34)
> ==31353== by 0x632EBB: readable_regcache::raw_read(int, unsigned char*) (regcache.c:538)
> ==31353== by 0x659D3F: riscv_return_value(gdbarch*, value*, type*, regcache*, unsigned char*, unsigned char const*) (riscv-tdep.c:2593)
> ==31353== by 0x583641: get_call_return_value (infcall.c:448)
> ==31353== by 0x583641: call_thread_fsm_should_stop(thread_fsm*, thread_info*) (infcall.c:546)
> ==31353== by 0x59BBEC: fetch_inferior_event(void*) (infrun.c:3883)
> ==31353== by 0x53890B: check_async_event_handlers (event-loop.c:1064)
> ==31353== by 0x53890B: gdb_do_one_event() [clone .part.4] (event-loop.c:326)
> ==31353== by 0x6CA34B: wait_sync_command_done() (top.c:503)
> ==31353== by 0x584653: run_inferior_call (infcall.c:621)
> ...
>
> And from gdb.base/call-sc.exp:
>
> (gdb) advance fun
> fun () at /gdb/gdb/testsuite/gdb.base/call-sc.c:41
> 41 return foo;
> (gdb) finish
> ==1968== Invalid write of size 8
> ==1968== at 0x4C34153: memmove (vg_replace_strmem.c:1270)
> ==1968== by 0x632EBB: memcpy (string_fortified.h:34)
> ==1968== by 0x632EBB: readable_regcache::raw_read(int, unsigned char*) (regcache.c:538)
> ==1968== by 0x659D01: riscv_return_value(gdbarch*, value*, type*, regcache*, unsigned char*, unsigned char const*) (riscv-tdep.c:2576)
> ==1968== by 0x5891E4: get_return_value(value*, type*) (infcmd.c:1640)
> ==1968== by 0x5892C4: finish_command_fsm_should_stop(thread_fsm*, thread_info*) (infcmd.c:1808)
> ==1968== by 0x59BBEC: fetch_inferior_event(void*) (infrun.c:3883)
> ==1968== by 0x53890B: check_async_event_handlers (event-loop.c:1064)
> ==1968== by 0x53890B: gdb_do_one_event() [clone .part.4] (event-loop.c:326)
> ==1968== by 0x6CA34B: wait_sync_command_done() (top.c:503)
> ...
>
> There are a couple of problems with the existing code, that are all
> related.
>
> In riscv_call_arg_struct we incorrectly rounded up the size of a
> structure argument. This is unnecessary, and caused GDB to read too
> much data into the output buffer when extracting a struct return
> value.
>
> In fixing this it became clear that we were incorrectly assuming that
> any value being placed in a register (or read from a register) would
> always access the entire register. This is not true, for example a
> 9-byte struct on a 64-bit target places 8-bytes in one registers and
> 1-byte in a second register (assuming available registers). To handle
> this I switch from using cooked_read to cooked_read_part.
>
> Finally, when processing basic integer return value types these are
> extended to xlen sized types and then passed in registers. We
> currently don't handle this type expansion in riscv_return_value, but
> we do in riscv_push_dummy_call. The result is that small integer
> types (like char) result in a full xlen sized register being written
> into the output buffer, which results in buffer overflow. To address
> this issue we now create a value of the expanded type and use this
> values contents buffer to hold the return value before casting the
> value down to the smaller expected type.
>
> This patch resolves all of the valgrind issues I have found so far,
> and causes no regressions. Tested against RV32/64 with and without
> floating point support.
>
> gdb/ChangeLog:
>
> * riscv-tdep.c (riscv_call_arg_struct): Don't adjust size before
> assigning locations.
> (riscv_return_value): Take more care not to read/write outside of
> argument buffer. Cast return value between the declared type and
> the abi type.
> ---
> gdb/ChangeLog | 8 +++++++
> gdb/riscv-tdep.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++------
> 2 files changed, 67 insertions(+), 6 deletions(-)
>
> diff --git a/gdb/riscv-tdep.c b/gdb/riscv-tdep.c
> index 5965a594440..ffbb9150c41 100644
> --- a/gdb/riscv-tdep.c
> +++ b/gdb/riscv-tdep.c
> @@ -2183,7 +2183,6 @@ riscv_call_arg_struct (struct riscv_arg_info *ainfo,
>
> /* Non of the structure flattening cases apply, so we just pass using
> the integer ABI. */
> - ainfo->length = align_up (ainfo->length, cinfo->xlen);
> riscv_call_arg_scalar_int (ainfo, cinfo);
> }
>
> @@ -2563,7 +2562,35 @@ riscv_return_value (struct gdbarch *gdbarch,
>
> if (readbuf != nullptr || writebuf != nullptr)
> {
> - int regnum;
> + unsigned int arg_len;
> + struct value *abi_val;
> + gdb_byte *old_readbuf = nullptr;
> + int regnum;
> +
> + /* We only do one thing at a time. */
> + gdb_assert (readbuf == nullptr || writebuf == nullptr);
> +
> + /* In some cases the argument is not returned as the declared type,
> + and we need to cast to or from the ABI type in order to
> + correctly access the argument. When writing to the machine we
> + do the cast here, when reading from the machine the cast occurs
> + later, after extracting the value. As the ABI type can be
> + larger than the declared type, then the read or write buffers
> + passed in might be too small. Here we ensure that we are using
> + buffers of sufficient size. */
> + if (writebuf != nullptr)
> + {
> + struct value *arg_val = value_from_contents (arg_type, writebuf);
> + abi_val = value_cast (info.type, arg_val);
> + writebuf = value_contents_raw (abi_val);
> + }
> + else
> + {
> + abi_val = allocate_value (info.type);
> + old_readbuf = readbuf;
> + readbuf = value_contents_raw (abi_val);
> + }
> + arg_len = TYPE_LENGTH (info.type);
>
> switch (info.argloc[0].loc_type)
> {
> @@ -2571,12 +2598,19 @@ riscv_return_value (struct gdbarch *gdbarch,
> case riscv_arg_info::location::in_reg:
> {
> regnum = info.argloc[0].loc_data.regno;
> + gdb_assert (info.argloc[0].c_length <= arg_len);
> + gdb_assert (info.argloc[0].c_length
> + <= register_size (gdbarch, regnum));
>
> if (readbuf)
> - regcache->cooked_read (regnum, readbuf);
> + regcache->cooked_read_part (regnum, 0,
> + info.argloc[0].c_length,
> + readbuf);
>
> if (writebuf)
> - regcache->cooked_write (regnum, writebuf);
> + regcache->cooked_write_part (regnum, 0,
> + info.argloc[0].c_length,
> + writebuf);
>
> /* A return value in register can have a second part in a
> second register. */
> @@ -2587,16 +2621,25 @@ riscv_return_value (struct gdbarch *gdbarch,
> case riscv_arg_info::location::in_reg:
> regnum = info.argloc[1].loc_data.regno;
>
> + gdb_assert ((info.argloc[0].c_length
> + + info.argloc[1].c_length) <= arg_len);
> + gdb_assert (info.argloc[1].c_length
> + <= register_size (gdbarch, regnum));
> +
> if (readbuf)
> {
> readbuf += info.argloc[1].c_offset;
> - regcache->cooked_read (regnum, readbuf);
> + regcache->cooked_read_part (regnum, 0,
> + info.argloc[1].c_length,
> + readbuf);
> }
>
> if (writebuf)
> {
> writebuf += info.argloc[1].c_offset;
> - regcache->cooked_write (regnum, writebuf);
> + regcache->cooked_write_part (regnum, 0,
> + info.argloc[1].c_length,
> + writebuf);
> }
> break;
>
> @@ -2629,6 +2672,16 @@ riscv_return_value (struct gdbarch *gdbarch,
> error (_("invalid argument location"));
> break;
> }
> +
> + /* This completes the cast from abi type back to the declared type
> + in the case that we are reading from the machine. See the
> + comment at the head of this block for more details. */
> + if (readbuf != nullptr)
> + {
> + struct value *arg_val = value_cast (arg_type, abi_val);
> + memcpy (old_readbuf, value_contents_raw (arg_val),
> + TYPE_LENGTH (arg_type));
> + }
> }
>
> switch (info.argloc[0].loc_type)
> --
> 2.14.5
>