Re: [PATCH] gdbstub.c: add support for info proc mappings

[Alex Bennée]

Dominik Czarnota <dominik.b.czarnota@gmail.com> writes:

> Hey,
>
> I may work on this next week but I will probably not make it until the 8th :(.
>
> On Thu, 3 Mar 2022 at 13:34, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>>
>> Disconnect3d <dominik.b.czarnota@gmail.com> writes:
>>
>> > This commit adds support for `info proc mappings` and a few other commands 
>> > into
>> > the QEMU user-mode emulation gdbstub.
>> >
>> > For that, support for the following GDB remote protocol commands has been 
>> > added:
>> > * vFile:setfs: pid
>> > * vFile:open: filename, flags, mode
>> > * vFile:pread: fd, count, offset
>> > * vFile:close: fd
>> > * qXfer:exec-file:read:annex:offset,length
>> >
<snip>
>> > +/*
>> > + *  Set to 1 to enable remote protocol debugging output. This output is 
>> > similar
>> > + *  to the one produced by the gdbserver's --remote-debug flag with some
>> > + *  additions. Anyway, the main debug prints are:
>> > + * - getpkt ("...") which refers to received data (or, send by the GDB 
>> > client)
>> > + * - putpkt ("...") which refers to sent data
>> > + */
>> > +#define ENABLE_REMOTE_DEBUG 0
>> > +
>> > +#if ENABLE_REMOTE_DEBUG
>> > +#define REMOTE_DEBUG_PRINT printf
>> > +#else
>> > +#define REMOTE_DEBUG_PRINT(...)
>> > +#endif
>>
>> We don't need this. The rest of the gdbstub has been instrumented with
>> tracepoints (try -d trace:gdbstub\* in your command line). See trace-events.
>>
>
> It would be convenient to support the gddbserver's --remote-debug flag
> but I understand it may not be wanted as it also means additional &
> unnecessary code in a production binary.
> I will remove this.

If you think the debug information will be useful by all means convert
the bits you need to tracepoints. 

>
>> > +
>> >  static inline int target_memory_rw_debug(CPUState *cpu, target_ulong addr,
>> >                                           uint8_t *buf, int len, bool 
>> > is_write)
>> >  {
>> > @@ -554,6 +573,7 @@ static int gdb_continue_partial(char *newstates)
>> >
>> >  static void put_buffer(const uint8_t *buf, int len)
>> >  {
>> > +    REMOTE_DEBUG_PRINT("putpkt (\"%.*s\");\n", len, buf);
>> >  #ifdef CONFIG_USER_ONLY
>> >      int ret;
>> >
>> > @@ -1982,6 +2002,157 @@ static void handle_v_kill(GArray *params, void 
>> > *user_ctx)
>> >      exit(0);
>> >  }
>> >
>> > +#ifdef CONFIG_USER_ONLY
>> > +/*
>> > + * Handles the `vFile:setfs: pid` command
>> > + *
>> > + * Example call: vFile:setfs:0
>> > + *
>> > + * --- From the GDB remote protocol documentation ---
>> > + * Select the filesystem on which vFile operations with filename arguments
>> > + * will operate. This is required for GDB to be able to access files on
>> > + * remote targets where the remote stub does not share a common 
>> > filesystem with
>> > + * the inferior(s). If pid is nonzero, select the filesystem as seen by 
>> > process
>> > + * pid. If pid is zero, select the filesystem as seen by the remote stub.
>> > + * Return 0 on success, or -1 if an error occurs. If vFile:setfs: 
>> > indicates
>> > + * success, the selected filesystem remains selected until the next 
>> > successful
>> > + * vFile:setfs: operation.
>> > +*/
>> > +static void handle_v_setfs(GArray *params, void *user_ctx)
>> > +{
>> > +    /*
>> > +     * We do not support different filesystem view for different pids
>> > +     * Return that all is OK, so that GDB can proceed
>> > +     */
>> > +    put_packet("F0");
>> > +}
>> > +
>> > +/*
>> > + * Handle the `vFile:open: filename, flags, mode` command
>> > + *
>> > + * We try to serve the filesystem here from the inferior point of view
>> > +
>> > + * Example call: vFile:open:6a7573742070726f62696e67,0,1c0
>> > + * (tries to open "just probing" with flags=0 mode=448)
>> > + *
>> > + * --- From the GDB remote protocol documentation ---
>> > + * Open a file at filename and return a file descriptor for it, or return
>> > + * -1 if an error occurs. The filename is a string, flags is an integer
>> > + * indicating a mask of open flags (see Open Flags), and mode is an 
>> > integer
>> > + * indicating a mask of mode bits to use if the file is created
>> > + * (see mode_t Values). See open, for details of the open flags and mode
>> > + * values.
>> > + */
>> > +static void handle_v_file_open(GArray *params, void *user_ctx)
>> > +{
>> > +    uint64_t flags = get_param(params, 1)->val_ull;
>> > +    uint64_t mode = get_param(params, 2)->val_ull;
>> > +    const char *hex_filename = get_param(params, 0)->data;
>> > +
>> > +    /* Decode the filename & append a null byte so we can use it later on 
>> > */
>> > +    hextomem(gdbserver_state.mem_buf, hex_filename, strlen(hex_filename));
>> > +    const char *null_byte = "\0";
>> > +    g_byte_array_append(gdbserver_state.mem_buf, (const guint8 
>> > *)null_byte, 1);
>> > +
>> > +    const char *filename = (const char *)gdbserver_state.mem_buf->data;
>> > +
>> > +    REMOTE_DEBUG_PRINT("vFile:open: filename=\"%s\" flags=%ld mode=%ld\n",
>> > +                       filename, flags, mode);
>> > +
>> > +    /*
>> > +     * On Linux we call the do_openat syscall on behalf of the inferior 
>> > as it
>> > +     * handles special filepaths properly like the /proc/$pid files, 
>> > which are
>> > +     * fetched by GDB for certain info (such as `info proc mappings`).
>> > +     */
>>
>> This sounds like a massive security hole to introduce without a
>> specific flag the user can explicitly enable. Semihosting has a similar
>> facility but also needs explicit configuration. Can we add a property to
>> the -gdb command line option to enable this:
>>
>>   -gdb tcp::1234,hostio=on
>>
>> ?
>
> It is, but should we really care? Both the execution and debugging of
> qemu-user emulation
> does not protect the host from arbitrary code execution and the
> proposed hostio=on flag will
> only make things less convenient and many people will just not use it
> due to not knowing that
> it exists.

Sure - but we are adding potential unexpected behaviour via the debug
stub not the guest program. The user may (although probably not) have
audited the binary they are running without realising the attack vector
the gdbstub introduces.

That said I take your point it's not super different from running
arbitrary guest code in linux-user. I think for system emulation this
functionality should definitely be an opt-in though.

Can we at least document the capability and potential security issues in
docs/system/gdb.rst?
>> > --- a/linux-user/syscall.c
>> > +++ b/linux-user/syscall.c
>> > @@ -8233,7 +8233,7 @@ static int open_hardware(void *cpu_env, int fd)
>> >  }
>> >  #endif
>> >
>> > -static int do_openat(void *cpu_env, int dirfd, const char *pathname, int 
>> > flags, mode_t mode)
>> > +int do_openat(void *cpu_env, int dirfd, const char *pathname, int
>> > flags, mode_t mode)
>>
>>
>> I wonder if this should be renamed to make the sense more clear?
>>
>> do_guest_openat? do_filtered_openat?
>>
>
> We can do, but its already in linux-user/syscall.c and I believe all
> syscalls there are executed
> "on behalf of guest".

Yes - but now you are exposing it to the wider qemu code base so a more
definitive name will prevent accidental misuse elsewhere.

>
>> >  {
>> >      struct fake_open {
>> >          const char *filename;
>>
>> Otherwise it looks fine. Do you have time to re-spin? I would need to
>> post a pull request with it by the 8th to make 7.0.
>>
>> --
>> Alex Bennée
>
> Thanks,
> Dominik 'disconnect3d' Czarnota


-- 
Alex Bennée