Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEATURE] Catch data also when syscalls like wrte fail #942

Closed
Andreagit97 opened this issue Mar 7, 2023 · 1 comment · Fixed by #980 or #985
Closed

[FEATURE] Catch data also when syscalls like wrte fail #942

Andreagit97 opened this issue Mar 7, 2023 · 1 comment · Fixed by #980 or #985
Assignees
Labels
kind/feature New feature or request
Milestone

Comments

@Andreagit97
Copy link
Member

Motivation

The original behavior of old drivers (kernel module, current bpf probe) was to catch the content of the buffer when syscalls like read/write failed. I've altered this behavior during the extension of the testing framework because relying on the user behavior seemed a little bit risky in my opinion. For example, when we call write in this way:

const unsigned data_len = 6;
char buf[data_len] = "hello";
ssize_t write_bytes = syscall(__NR_write, -1, (void *)buf, 800);

our drivers collect (800-6) bytes of junk if the space they are reading is mapped in memory, obtaining something like this:

hello�t���z��X�yU0\+�y�z�yU�*d�yU�k`�yU����yU�

See the full example here: #900 (comment)
For this reason, I chose to send empty params to userspace when these syscalls fail!

Talking with @LucaGuerra @loresuso (thanks folks!) they pointed out that it could be useful to get the full buffer for troubleshooting purposes even if these syscalls fail. We cannot cause "invalid pointer" crashes because all our drivers check the memory with the access_ok method before reading from the pointer.

(bpf_probe_read_user implementation 👇 )

long copy_from_user_nofault(void *dst, const void __user *src, size_t size)
{
	long ret = -EFAULT;
	if (access_ok(src, size)) {
		pagefault_disable();
		ret = __copy_from_user_inatomic(dst, src, size);
		pagefault_enable();
	}

	if (ret)
		return -EFAULT;
	return 0;
}

For this reason, I would revert all modifications to old drivers to restore the original behavior and I will adapt also the modern probe to do the same stuff. I will open a PR to address these changes as explained here #900 (comment)

@Andreagit97
Copy link
Member Author

Andreagit97 commented Mar 10, 2023

This was the initial situation before I introduced the driver tests framework. As you can see there were already some inconsistencies:
(N) means that data are not collected when the syscall fails
(Y) means that data are collected also when the syscall fails

  • read (N)
  • recvfrom (N)
  • recvmsg (Y)
  • sendmsg (Y)
  • sendto (N)
  • write (Y)
  • send (N)
  • recv (N)
  • writev (Y)
  • pwrite64 (Y)
  • pwritev (Y)
  • pread64 (Y)
  • preadv (Y)
  • readv (Y)

Probably what we want here is to obtain data when the syscall fails only with write family syscalls. This is because when a read fails the kernel doesn't fill the data so we cannot collect anything except the destination memory area that the user provided to the read, but probably this is not what we want :/ WDYT? So the final solution after the refactor would be something like:

  • read (N)
  • recvfrom (N)
  • recvmsg (N)
  • sendmsg (Y)
  • sendto (Y)
  • write (Y)
  • send (Y)
  • recv (N)
  • writev (Y)
  • pwrite64 (Y)
  • pwritev (Y)
  • pread64 (N)
  • preadv (N)
  • readv (N)

@Andreagit97 Andreagit97 self-assigned this Mar 10, 2023
@Andreagit97 Andreagit97 changed the title [FEATURE] Catch data also when syscalls like wrte/read fail [FEATURE] Catch data also when syscalls like wrte fail Mar 14, 2023
@Andreagit97 Andreagit97 reopened this Mar 17, 2023
@Andreagit97 Andreagit97 added this to the 0.11.0 milestone Mar 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment