* [PATCH v2] gdbserver: linux_low: elf_64_file_p cache results
@ 2017-08-24 4:45 jon
2017-08-24 9:27 ` Pedro Alves
0 siblings, 1 reply; 6+ messages in thread
From: jon @ 2017-08-24 4:45 UTC (permalink / raw)
To: gdb-patches; +Cc: Jon Ringle
From: Jon Ringle <jringle@gridpoint.com>
I was debugging a systemd service that would drop root privileges, and
found that I was unable to debug it properly because gdb could not resolve
any symbols. The gdbserver on my arm target would report:
gdbserver: Corrupted shared library list: 0x0 != 0xe2822038
Prior to getting the above message, symbols could be resolved successfully.
I finally determined that the problem was that in linux_low.c in function
linux_qxfer_libraries_svr4() the is_elf64 variable was causing the wrong
lmo and ptr_size to get used. Here's that code snippet:
7136 static int
7137 linux_qxfer_libraries_svr4 (const char *annex, unsigned char *readbuf,
7138 unsigned const char *writebuf,
7139 CORE_ADDR offset, int len)
7140 {
7141 char *document;
7142 unsigned document_len;
7143 struct process_info_private *const priv = current_process ()->priv;
7144 char filename[PATH_MAX];
7145 int pid, is_elf64;
7146
7147 static const struct link_map_offsets lmo_32bit_offsets =
7148 {
7149 0, /* r_version offset. */
7150 4, /* r_debug.r_map offset. */
7151 0, /* l_addr offset in link_map. */
7152 4, /* l_name offset in link_map. */
7153 8, /* l_ld offset in link_map. */
7154 12, /* l_next offset in link_map. */
7155 16 /* l_prev offset in link_map. */
7156 };
7157
7158 static const struct link_map_offsets lmo_64bit_offsets =
7159 {
7160 0, /* r_version offset. */
7161 8, /* r_debug.r_map offset. */
7162 0, /* l_addr offset in link_map. */
7163 8, /* l_name offset in link_map. */
7164 16, /* l_ld offset in link_map. */
7165 24, /* l_next offset in link_map. */
7166 32 /* l_prev offset in link_map. */
7167 };
7168 const struct link_map_offsets *lmo;
7169 unsigned int machine;
7170 int ptr_size;
7171 CORE_ADDR lm_addr = 0, lm_prev = 0;
7172 int allocated = 1024;
7173 char *p;
7174 CORE_ADDR l_name, l_addr, l_ld, l_next, l_prev;
7175 int header_done = 0;
7176
7177 if (writebuf != NULL)
7178 return -2;
7179 if (readbuf == NULL)
7180 return -1;
7181
7182 pid = lwpid_of (current_thread);
7183 xsnprintf (filename, sizeof filename, "/proc/%d/exe", pid);
7184 is_elf64 = elf_64_file_p (filename, &machine);
7185 lmo = is_elf64 ? &lmo_64bit_offsets : &lmo_32bit_offsets;
7186 ptr_size = is_elf64 ? 8 : 4;
The problem lied in the fact that the function elf_64_file_p() (call on
line 7184), was returning -1 because it could not open the file (with errno
set to EACCESS). This was because the inferior's code was dropping root
privileges and no longer was able to read the file.
This patch implements a cache per file in the elf_64_file_p() function so
that it remembers the results of a previous query for the same filename.
Since it seems that c++ is now accepted in gdb, the cache was implemented
using std::map
---
gdb/gdbserver/linux-low.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index fd46d85..cb3faa6 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -50,6 +50,8 @@
#include "common-inferior.h"
#include "nat/fork-inferior.h"
#include "environ.h"
+#include <string>
+#include <map>
#ifndef ELFMAG0
/* Don't include <linux/elf.h> here. If it got included by gdb_proc_service.h
then ELFMAG0 will have been defined. If it didn't get included by
@@ -373,24 +375,33 @@ elf_64_header_p (const Elf64_Ehdr *header, unsigned int *machine)
zero if the file is not a 64-bit ELF file,
and -1 if the file is not accessible or doesn't exist. */
+static std::map<std::string, std::pair<int, unsigned int>> is_elf64_cache;
+
static int
elf_64_file_p (const char *file, unsigned int *machine)
{
Elf64_Ehdr header;
int fd;
+ int is_elf64 = 0;
+
+ auto iter = is_elf64_cache.find(file);
+ if (iter != is_elf64_cache.end())
+ {
+ *machine = iter->second.second;
+ return iter->second.first;
+ }
fd = open (file, O_RDONLY);
if (fd < 0)
return -1;
- if (read (fd, &header, sizeof (header)) != sizeof (header))
- {
- close (fd);
- return 0;
- }
+ if (read (fd, &header, sizeof (header)) == sizeof (header))
+ is_elf64 = elf_64_header_p(&header, machine);
+
close (fd);
- return elf_64_header_p (&header, machine);
+ is_elf64_cache.emplace(file, std::make_pair(is_elf64, *machine));
+ return is_elf64;
}
/* Accepts an integer PID; Returns true if the executable PID is
--
1.9.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] gdbserver: linux_low: elf_64_file_p cache results
2017-08-24 4:45 [PATCH v2] gdbserver: linux_low: elf_64_file_p cache results jon
@ 2017-08-24 9:27 ` Pedro Alves
2017-08-24 14:15 ` Jon Ringle
0 siblings, 1 reply; 6+ messages in thread
From: Pedro Alves @ 2017-08-24 9:27 UTC (permalink / raw)
To: jon, gdb-patches; +Cc: Jon Ringle
Hi Jon,
On 08/24/2017 05:45 AM, jon@ringle.org wrote:
> The problem lied in the fact that the function elf_64_file_p() (call on
> line 7184), was returning -1 because it could not open the file (with errno
> set to EACCESS). This was because the inferior's code was dropping root
> privileges and no longer was able to read the file.
I feel like I'm missing something. If it's the inferior that
is dropping privileges, how can that affect gdbserver? It's gdbserver
that opens the file, not the inferior.
It'd be nice to have a testcase for this. Would it be possible to come
up with a small reproducer?
> This patch implements a cache per file in the elf_64_file_p() function so
> that it remembers the results of a previous query for the same filename.
>
> Since it seems that c++ is now accepted in gdb, the cache was implemented
> using std::map
Doesn't look correct, since it assumes that the 64-bit-ness
of "/proc/PID/exe" stays constant for the lifetime of gdbserver,
which is certainly false. With "gdbserver --multi", a single gdbserver can
stay around debugging multiple processes for an undeterminate amount of time.
After the current process PID exits, nothing prevents the kernel from
reusing PID for another process.
Also, we need to clear the cache when the inferior process execs,
since a 32-bit process can well exec a 64-bit process, and vice-versa.
If caching is the right approach, then it seems to me that it'd be
much simpler/efficient to make it a new 'bool is_elf64;' field of
struct process_info_private.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] gdbserver: linux_low: elf_64_file_p cache results
2017-08-24 9:27 ` Pedro Alves
@ 2017-08-24 14:15 ` Jon Ringle
2017-08-24 14:42 ` Pedro Alves
0 siblings, 1 reply; 6+ messages in thread
From: Jon Ringle @ 2017-08-24 14:15 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches, Jon Ringle
On Thu, Aug 24, 2017 at 5:26 AM, Pedro Alves <palves@redhat.com> wrote:
> Hi Jon,
>
> On 08/24/2017 05:45 AM, jon@ringle.org wrote:
>
>> The problem lied in the fact that the function elf_64_file_p() (call on
>> line 7184), was returning -1 because it could not open the file (with errno
>> set to EACCESS). This was because the inferior's code was dropping root
>> privileges and no longer was able to read the file.
>
> I feel like I'm missing something. If it's the inferior that
> is dropping privileges, how can that affect gdbserver? It's gdbserver
> that opens the file, not the inferior.
>
> It'd be nice to have a testcase for this. Would it be possible to come
> up with a small reproducer?
>
I learned something while coming up with a small reproducer. The real
program I'm debugging is a systemd service and has a
CapabilityBoundingSet. The CapabilityBoundingSet turns out to be a
crucial point for reproducing.
I setup a systemd service:
$ cat /lib/systemd/system/droproot-test.service
[Unit]
Description=gdbserver droproot test service
[Service]
Type=simple
ExecStart=/home/jringle/build/gdbserver/gdbserver :5555
/home/jringle/git/droproot-test/droproot-test jringle
Restart=on-success
CapabilityBoundingSet=CAP_SETGID CAP_SETUID
Here's droproot-test.c:
$ cat -n droproot-test.c
1 #include <errno.h>
2 #include <grp.h>
3 #include <pwd.h>
4 #include <stdio.h>
5 #include <stdlib.h>
6 #include <string.h>
7 #include <sys/types.h>
8 #include <unistd.h>
9
10 /* Drop root privileges and chroot if necessary */
11 void droproot(const char *username, const char *chroot_dir)
12 {
13 struct passwd *pw = NULL;
14
15 if (!username)
16 {
17 fprintf(stderr, "%s: no username given", __func__);
18 return;
19 }
20
21 if (chroot_dir && !username)
22 {
23 fprintf(stderr, "%s: Chroot without dropping root is
insecure\n", __func__);
24 exit(1);
25 }
26
27 pw = getpwnam(username);
28 if (pw)
29 {
30 if (chroot_dir)
31 {
32 if (chroot(chroot_dir) != 0 || chdir ("/") != 0)
33 {
34 fprintf(stderr, "%s: Couldn't chroot/chdir to
'%.64s': %s\n", __func__,
35 chroot_dir, strerror(errno));
36 exit(1);
37 }
38 }
39 if (initgroups(pw->pw_name, pw->pw_gid) != 0 ||
40 setgid(pw->pw_gid) != 0 || setuid(pw->pw_uid) != 0)
41 {
42 fprintf(stderr, "%s: Couldn't change to '%.32s'
uid=%lu gid=%lu: %s\n", __func__,
43 username,
44 (unsigned long)pw->pw_uid,
45 (unsigned long)pw->pw_gid,
46 strerror(errno));
47 exit(1);
48 }
49 }
50 else
51 {
52 fprintf(stderr, "%s: Couldn't find user '%.32s'\n", __func__,
53 username);
54 exit(1);
55 }
56 }
57
58 int main(int argc, char *argv[])
59 {
60 int uid = getuid();
61
62 printf("Before droproot uid is %d\n", uid);
63 fflush(stdout);
64 if (uid == 0)
65 {
66 droproot(argv[1], NULL);
67 }
68
69 uid = getuid();
70 printf("After droproot uid is %d\n", uid);
71 fflush(stdout);
72
73 return 0;
74 }
If you break on line 66, gdbserver will be able to read the r_debug
table just fine, but step to next line after root has been dropped, it
can't:
$ gdb droproot-test
GNU gdb (Ubuntu 7.11.1-0ubuntu1~16.5) 7.11.1
Copyright (C) 2016 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later
<http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law. Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>.
Find the GDB manual and other documentation resources online at:
<http://www.gnu.org/software/gdb/documentation/>.
For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from droproot-test...done.
(gdb) target remote :5555
Remote debugging using :5555
Reading /lib/ld-linux.so.2 from remote target...
warning: File transfers from remote targets can be slow. Use "set
sysroot" to access files locally instead.
Reading /lib/ld-linux.so.2 from remote target...
Reading symbols from target:/lib/ld-linux.so.2...Reading
/lib/b9332ff9d20717877cd3ff483d9426825cdef5.debug from remote
target...
Reading /lib/.debug/b9332ff9d20717877cd3ff483d9426825cdef5.debug
from remote target...
(no debugging symbols found)...done.
0xf7fd9a20 in ?? () from target:/lib/ld-linux.so.2
(gdb) tb 66
Temporary breakpoint 1 at 0x804861f: file droproot-test.c, line 66.
(gdb) cont
Continuing.
Reading /lib32/libc.so.6 from remote target...
Reading /lib32/333186c6b532511a68d16aca4c61422eb772da.debug from
remote target...
Reading /lib32/.debug/333186c6b532511a68d16aca4c61422eb772da.debug
from remote target...
Temporary breakpoint 1, main (argc=2, argv=0xffffde54) at droproot-test.c:66
66 droproot(argv[1], NULL);
(gdb) info sharedlibrary
From To Syms Read Shared Object Library
0xf7fd9860 0xf7ff271d Yes (*) target:/lib/ld-linux.so.2
0xf7e18750 0xf7f4186d Yes (*) target:/lib32/libc.so.6
(*): Shared library is missing debugging information.
(gdb) n
Reading /lib32/libnss_compat.so.2 from remote target...
Reading /lib32/libnsl.so.1 from remote target...
Reading /lib32/6a2d2c3683ee8c596fb834c373a010a527cf23.debug from
remote target...
Reading /lib32/.debug/6a2d2c3683ee8c596fb834c373a010a527cf23.debug
from remote target...
Reading /lib32/b31e7cb309897e1db3b4abc464de2e324e5606.debug from
remote target...
Reading /lib32/.debug/b31e7cb309897e1db3b4abc464de2e324e5606.debug
from remote target...
Reading /lib32/libnss_nis.so.2 from remote target...
Reading /lib32/libnss_files.so.2 from remote target...
Reading /lib32/3730cbd17cd03a91793441cf426924c3efe048.debug from
remote target...
Reading /lib32/.debug/3730cbd17cd03a91793441cf426924c3efe048.debug
from remote target...
Reading /lib32/54fdb9da826ae5d6cbc2554bbadd512b7b639d.debug from
remote target...
Reading /lib32/.debug/54fdb9da826ae5d6cbc2554bbadd512b7b639d.debug
from remote target...
69 uid = getuid();
(gdb) info sharedlibrary
From To Syms Read Shared Object Library
0xf7fd9860 0xf7ff271d Yes (*) target:/lib/ld-linux.so.2
(*): Shared library is missing debugging information.
(gdb)
Here's the gdbserver output:
$ sudo systemctl restart droproot-test && journalctl -o cat -u
droproot-test -f
Started gdbserver droproot test service.
Process /home/jringle/git/droproot-test/droproot-test created; pid = 18180
Listening on port 5555
Remote debugging from host 127.0.0.1
Before droproot uid is 0
gdbserver: Corrupted shared library list: 0x0 != 0x580b8c8d4cca6b
>> This patch implements a cache per file in the elf_64_file_p() function so
>> that it remembers the results of a previous query for the same filename.
>>
>> Since it seems that c++ is now accepted in gdb, the cache was implemented
>> using std::map
>
> Doesn't look correct, since it assumes that the 64-bit-ness
> of "/proc/PID/exe" stays constant for the lifetime of gdbserver,
> which is certainly false. With "gdbserver --multi", a single gdbserver can
> stay around debugging multiple processes for an undeterminate amount of time.
> After the current process PID exits, nothing prevents the kernel from
> reusing PID for another process.
>
> Also, we need to clear the cache when the inferior process execs,
> since a 32-bit process can well exec a 64-bit process, and vice-versa.
>
> If caching is the right approach, then it seems to me that it'd be
> much simpler/efficient to make it a new 'bool is_elf64;' field of
> struct process_info_private.
That sounds like a good idea. I'll look into implementing something
along those lines...
-Jon
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] gdbserver: linux_low: elf_64_file_p cache results
2017-08-24 14:15 ` Jon Ringle
@ 2017-08-24 14:42 ` Pedro Alves
2017-08-24 14:53 ` Pedro Alves
0 siblings, 1 reply; 6+ messages in thread
From: Pedro Alves @ 2017-08-24 14:42 UTC (permalink / raw)
To: Jon Ringle; +Cc: gdb-patches, Jon Ringle
On 08/24/2017 03:14 PM, Jon Ringle wrote:
> On Thu, Aug 24, 2017 at 5:26 AM, Pedro Alves <palves@redhat.com> wrote:
>> Hi Jon,
>>
>> On 08/24/2017 05:45 AM, jon@ringle.org wrote:
>>
>>> The problem lied in the fact that the function elf_64_file_p() (call on
>>> line 7184), was returning -1 because it could not open the file (with errno
>>> set to EACCESS). This was because the inferior's code was dropping root
>>> privileges and no longer was able to read the file.
>>
>> I feel like I'm missing something. If it's the inferior that
>> is dropping privileges, how can that affect gdbserver? It's gdbserver
>> that opens the file, not the inferior.
>>
>> It'd be nice to have a testcase for this. Would it be possible to come
>> up with a small reproducer?
>>
>
> I learned something while coming up with a small reproducer. The real
> program I'm debugging is a systemd service and has a
> CapabilityBoundingSet. The CapabilityBoundingSet turns out to be a
> crucial point for reproducing.
>
> I setup a systemd service:
> $ cat /lib/systemd/system/droproot-test.service
> [Unit]
> Description=gdbserver droproot test service
>
> [Service]
> Type=simple
> ExecStart=/home/jringle/build/gdbserver/gdbserver :5555
> /home/jringle/git/droproot-test/droproot-test jringle
> Restart=on-success
> CapabilityBoundingSet=CAP_SETGID CAP_SETUID
OK, I don't know much about this, but it sounds like
without that, your "droproot" test wouldn't be able
to call setgid/setuid successfully to change to the
"jringle" user.
I'm still mystified about why can't gdbserver read
the file after "droproot" has changed user.
I assume gdbserver is running as root? Why wouldn't
a gdbserver running as root be able to read "jringle"'s
/proc file?
Does CAP_PTRACE make a difference?
I have to wonder whether there's a better way to do this..
gdbserver needs to read other /proc files, some not cacheable.
I fear that you may have run into just one case so far, and
that we may run into problems if we take this route.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] gdbserver: linux_low: elf_64_file_p cache results
2017-08-24 14:42 ` Pedro Alves
@ 2017-08-24 14:53 ` Pedro Alves
2017-08-24 15:00 ` Jon Ringle
0 siblings, 1 reply; 6+ messages in thread
From: Pedro Alves @ 2017-08-24 14:53 UTC (permalink / raw)
To: Jon Ringle; +Cc: gdb-patches, Jon Ringle
On 08/24/2017 03:42 PM, Pedro Alves wrote:
> I'm still mystified about why can't gdbserver read
> the file after "droproot" has changed user.
> I assume gdbserver is running as root? Why wouldn't
> a gdbserver running as root be able to read "jringle"'s
> /proc file?
>
> Does CAP_PTRACE make a difference?
FAOD, I meant CAP_SYS_PTRACE.
See for example here:
http://man7.org/linux/man-pages/man5/proc.5.html
~~~
/proc/[pid]/exe
...
Permission to dereference or read (readlink(2)) this symbolic
link is governed by a ptrace access mode
PTRACE_MODE_READ_FSCREDS check; see ptrace(2).
~~~
[and follow on to ptrace(2).]
>
> I have to wonder whether there's a better way to do this..
> gdbserver needs to read other /proc files, some not cacheable.
> I fear that you may have run into just one case so far, and
> that we may run into problems if we take this route.
Thanks,
Pedro Alves
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] gdbserver: linux_low: elf_64_file_p cache results
2017-08-24 14:53 ` Pedro Alves
@ 2017-08-24 15:00 ` Jon Ringle
0 siblings, 0 replies; 6+ messages in thread
From: Jon Ringle @ 2017-08-24 15:00 UTC (permalink / raw)
To: Pedro Alves; +Cc: gdb-patches, Jon Ringle
On Thu, Aug 24, 2017 at 10:53 AM, Pedro Alves <palves@redhat.com> wrote:
> On 08/24/2017 03:42 PM, Pedro Alves wrote:
>
>> I'm still mystified about why can't gdbserver read
>> the file after "droproot" has changed user.
>> I assume gdbserver is running as root? Why wouldn't
>> a gdbserver running as root be able to read "jringle"'s
>> /proc file?
>>
>> Does CAP_PTRACE make a difference?
>
> FAOD, I meant CAP_SYS_PTRACE.
Yes, it does make a difference. My testcase works if I add
CAP_SYS_PTRACE to the CapabilityBoundingSet :)
-Jon
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-08-24 15:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-24 4:45 [PATCH v2] gdbserver: linux_low: elf_64_file_p cache results jon
2017-08-24 9:27 ` Pedro Alves
2017-08-24 14:15 ` Jon Ringle
2017-08-24 14:42 ` Pedro Alves
2017-08-24 14:53 ` Pedro Alves
2017-08-24 15:00 ` Jon Ringle
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).