Execve ttyname #976

arossert · 2017-11-07T09:21:54Z

this will add the TTY name (like /dev/pts/10) at execve events in continue to #792 $ #918

extract TTY name using the existing ppm_get_tty, not sure that this is the best approach.
build TTY name using snprintf and information from tty_struct and tty_driver
if TTY can't be extract ? will be placed instead (like when using ps -j).
read TTY value at startup from /proc using stdin file descriptor.
save/read tty and ttyname to/from sacp file.
added filter to ttyname field.
tested on a container by executing docker run -it ubuntu bash and then run tty command in the bash inside the container (got the same value in the execve event from sysdig).

changed old `tty` argument to `tty_nr` added new `tty` string argument

sysdig-CLA-1.0-contributing-entity: Amir Rossert sysdig-CLA-1.0-signed-off-by: John Tsai johntsai@paypal.com

mstemm · 2017-11-07T21:26:22Z

I think I'd prefer extracting the tty name in userspace instead of the driver. That way you don't need to change the execve event or fillers. You could look it up right after reading from proc in scap_proc_fill_info_from_stats, or after reading the tty from the execve event.

Btw, thanks for properly saving the tty in scap files, I noticed that was missing.

arossert · 2017-11-08T07:30:03Z

@mstemm the way I see it is that the value that for tty (that is actually tty_nr) in the current version is not that usable, from /proc/[pid]/stat man page the tty_nr value is:

The controlling terminal of the process. (The minor device number is contained in the combination of bits 31 to 20 and 7 to 0; the major device number is in bits 15 to 8.)

To do the translation of the tty_nr value to its human readable value like tty process does, we need to do something like what I have described in #918 for evrey execve exit event.

The human-readable value for the tty can be extracted from the kernel as easily as the tty_nr value (look at ppm_get_tty() function).

In my opinion, the current tty value can be replaced by this value because it is much more usable.

arossert · 2017-11-16T18:33:31Z

@mstemm any suggestion regarding this PR?
I would really like to see this feature implemented.

mstemm · 2017-11-30T01:29:16Z

Ok, that seems reasonable. I also wanted to make sure that the tty_nr maps to the right major/minor for processes running in a container, and they do.

@ret2libc could you also take a look at these changes before we merge?

ret2libc · 2017-11-30T10:50:34Z

driver/ppm_fillers.c

+
+		if (tty_nr > 0)
+			res = val_to_ring(args, (int64_t)(long)args->str_storage, 0, false, 0);
+		else // if the tty_nr is not found put `?` like in ps


Use comments like in other places with /* ... */

ret2libc · 2017-11-30T10:58:53Z

driver/ppm_fillers.c

@@ -1184,6 +1186,9 @@ static int ppm_get_tty(void)

 	tty_nr = new_encode_dev(MKDEV(major, minor_start) + index);

+	if (snprintf(buf, buflen, "/dev/%s/%d", driver->name, index) < 0)


I may be wrong, but I think this could cause problems because you are dereferencing a pointer without doing the probe_kernel_read and it could fault the kernel in this particular context. @gianlucaborello ?

Can you tell me how we can use probe_kernel_read in this case?

There is an option to get this value as one string from struct tty_struct, it has a data member char name[64] that holds the value as <driver><index> like pst12 and then build the full path in the userspace.

It could work. If you get the name from tty_struct at least you know what is the size to use with probe_kernel_read.

ret2libc · 2017-11-30T11:05:41Z

driver/ppm_fillers.c

@@ -1184,6 +1186,9 @@ static int ppm_get_tty(void)

 	tty_nr = new_encode_dev(MKDEV(major, minor_start) + index);

+	if (snprintf(buf, buflen, "/dev/%s/%d", driver->name, index) < 0)
+		strncpy(buf, "<NA>", buflen);


Could it fail too?
Are we sure that buf is always initialized with a valid string? If that's not the case, we could run into issues when later we call val_to_ring

ret2libc · 2017-11-30T11:07:45Z

userspace/libscap/scap_savefile.c

@@ -316,7 +318,10 @@ int32_t scap_write_proclist_entry(scap_t *handle, scap_dumper_t *d, struct scap_
 		    scap_dump_write(d, &(tinfo->cgroups_len), sizeof(uint16_t)) != sizeof(uint16_t) ||
 		    scap_dump_write(d, tinfo->cgroups, tinfo->cgroups_len) != tinfo->cgroups_len ||
 		    scap_dump_write(d, &rootlen, sizeof(uint16_t)) != sizeof(uint16_t) ||
-		    scap_dump_write(d, tinfo->root, rootlen) != rootlen)
+		    scap_dump_write(d, tinfo->root, rootlen) != rootlen ||
+			scap_dump_write(d, &(tinfo->tty), sizeof(int32_t)) != sizeof(int32_t) ||


wrong indentation here and on the next line

ret2libc · 2017-11-30T14:18:20Z

userspace/libsinsp/threadinfo.cpp

@@ -1439,7 +1442,8 @@ void sinsp_thread_manager::dump_threads_to_file(scap_dumper_t* dumper)
 			sizeof(int64_t) +  // vtid
 			sizeof(int64_t) +  // vpid
 			2 + sctinfo->cgroups_len +
-			sizeof(uint32_t) +
+			sizeof(uint32_t) + // tty
+		    2 + MIN(tinfo.m_ttyname.size(), SCAP_MAX_PATH_SIZE) +


Fix indentation

ret2libc · 2017-11-30T14:47:14Z

driver/ppm_fillers.c

@@ -1184,6 +1186,9 @@ static int ppm_get_tty(void)

 	tty_nr = new_encode_dev(MKDEV(major, minor_start) + index);

+	if (snprintf(buf, buflen, "/dev/%s/%d", driver->name, index) < 0)


Is this always right? What about all those cases written in #918 (comment) ?

I have checked all the cases and got the same output as tty process

On my machine I get /dev/tty1 and that cannot be generated with this fmt string.

I have tested it on Ubuntu/RHEL and in both I get the name as above /dev/pts/6.
On my mac OSX I get this /dev/ttys001 but mac is not supported.

@gianlucaborello said that the full path is preferred but I can change it to the short version

@arossert Are you connecting to them through SSH or similar? I get /dev/pts/X in that case, but if you go to the real terminal by using CTRL-ALT-F1 and do tty you get /dev/tty1

Since you can't find a complete match in kernel space, I think I'm back to suggesting it be done in userspace. You can do the /sys/dev/... lookup in userspace to match the major/minor number and construct the full path from there.

I'm not sure how we can lookup the tty name from /sys/dev/.
And if we will do it in the userspace, it will not appear in the syscall arguments, correct?

What I was originally thinking is that from tty_nr you can get the device major and minor number. https://stackoverflow.com/questions/19598467/getting-minor-device-number-from-tty-nr-attribute-in-proc-pid-stat points out the macros to use.

Once you have the major/minor number, you can look at /sys/dev/{block,char}/{major}:{minor}, which is a symlink to some directory below /sys/devices/.... In that directory, the file uevent has a DEVNAME field, which is the path below /dev for the device.

For pseudo-ttys aka /dev/pts/N, it's a bit more complicated, at those devices don't appear below /sys/dev. For those, you could also look at the devices /dev/pts/NNN, get their device numbers, and try to match them to the major/minor device numbers encoded in tty_nr.

I see that if you combine the tty_driver.name and tty_driver.index you do get 'pts/NNN`, so maybe that's good enough after all. You would still have to work out the discrepancy between pseudo tty and actual tty devices.

It looks very complicated task if we are willing to use the TTY name as it appears in the kernel (like pts12) I can fix that.

Did you take a look at how procps translate the tty_nr to the name?
Do you think we can adopt this code?

Of the 4 methods procps tries, driver_name seems very lightweight and likely to work most of the time, with entries to map device major/minor to a full /dev/xxxNNN name. It seems to handle the most common terminal types. How about you adapt the approach of reading /proc/tty/drivers (but not the code directly, that code is a different license than sysdig's), in userspace, from the tty_nr ?

sysdig-CLA-1.0-contributing-entity: Amir Rossert sysdig-CLA-1.0-signed-off-by: John Tsai johntsai@paypal.com

github-actions · 2023-02-23T02:04:43Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

arossert added 4 commits October 31, 2017 17:00

Added TTY value as human readable value

d81deb3

changed old `tty` argument to `tty_nr` added new `tty` string argument

Fixes

316cf68

add tty as human readable string

812aeaf

sysdig-CLA-1.0-contributing-entity: Amir Rossert sysdig-CLA-1.0-signed-off-by: John Tsai johntsai@paypal.com

put ? in ttyname if stdin is not available

2034c81

sysdig-CLA-1.0-contributing-entity: Amir Rossert sysdig-CLA-1.0-signed-off-by: John Tsai johntsai@paypal.com

ret2libc suggested changes Nov 30, 2017

View reviewed changes

fix indentation and comments

a465ff3

sysdig-CLA-1.0-contributing-entity: Amir Rossert sysdig-CLA-1.0-signed-off-by: John Tsai johntsai@paypal.com

github-actions bot added the stale label Feb 23, 2023

github-actions bot closed this Mar 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Execve ttyname #976

Execve ttyname #976

arossert commented Nov 7, 2017 •

edited

Loading

mstemm commented Nov 7, 2017

arossert commented Nov 8, 2017

arossert commented Nov 16, 2017

mstemm commented Nov 30, 2017

ret2libc Nov 30, 2017

ret2libc Nov 30, 2017

arossert Nov 30, 2017

ret2libc Nov 30, 2017

ret2libc Nov 30, 2017

ret2libc Nov 30, 2017

ret2libc Nov 30, 2017

ret2libc Nov 30, 2017

arossert Nov 30, 2017

ret2libc Nov 30, 2017

arossert Nov 30, 2017

ret2libc Nov 30, 2017

mstemm Dec 6, 2017

arossert Dec 6, 2017

mstemm Dec 6, 2017

arossert Dec 7, 2017

mstemm Dec 12, 2017

github-actions bot commented Feb 23, 2023

		@@ -1184,6 +1186,9 @@ static int ppm_get_tty(void)

		tty_nr = new_encode_dev(MKDEV(major, minor_start) + index);

		if (snprintf(buf, buflen, "/dev/%s/%d", driver->name, index) < 0)

Execve ttyname #976

Execve ttyname #976

Conversation

arossert commented Nov 7, 2017 • edited Loading

mstemm commented Nov 7, 2017

arossert commented Nov 8, 2017

arossert commented Nov 16, 2017

mstemm commented Nov 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 23, 2023

arossert commented Nov 7, 2017 •

edited

Loading