Skip to content

Fix inconsistent reg struct layout for 64bit tracer and 32bit tracee #2448

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.

Sign up for GitHub

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fix inconsistent reg struct layout for 64bit tracer and 32bit tracee #2448

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
facf00c
Fix inconsistent reg struct layout for 64bit tracer and 32bit tracee
Evian-Zhang Jun 14, 2024
db97950
Mark getregs as unsafe. Encourage to use getregset for safe version
Evian-Zhang Jun 15, 2024
79c3093
Fix test failed due to the unsafe change of getregs
Evian-Zhang Jun 15, 2024
581857b
Fix Safety section missing for unsafe getregs
Evian-Zhang Jun 15, 2024
8de8124
Fix broken documentation link
Evian-Zhang Jun 15, 2024
45466fd
Mention EIO error when getregset have inconsistent arch between trace…
Evian-Zhang Jun 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/sys/ptrace/linux.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,9 @@ fn ptrace_peek(
/// `ptrace(PTRACE_GETREGSET, pid, NT_PRSTATUS, ...)` is used instead to achieve the same effect
/// on aarch64 and riscv64.
///
/// Currently, in x86_64 platform, if the tracer is 64bit and tracee is 32bit, this function
/// will return an error.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it? Did you forget to add a size check below?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, I forgot this check. But as stated in the man page, PTRACE_GETREGS does not return the size of bytes written, so it seems that there is no easy way to check this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you know how C programs use PTRACE_GETREGS for 32-bit targets, then?

Copy link
Author

@Evian-Zhang Evian-Zhang Jun 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know, we should check the target tracee bitness in advance, instead of checking after the syscall returns. Is it better to mark this method unsafe and encourage users to use getregset for safe tracee register retrieval?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it cannot guarantee that the return value will be correctly constructed, then yes it must be unsafe. Either that, or it could return a union (which is unsafe to read from).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. I prefer to mark it as unsafe, since it is hard to import the 32bit user_regs_struct in libc crate.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know, we should check the target tracee bitness in advance, instead of checking after the syscall returns.

From this post, it is not trivial to check the bitness, so I would also prefer to mark this function unsafe as well if we don't have a better option.

since it is hard to import the 32bit user_regs_struct in libc crate

Would you like to elaborate on this a bit?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we return a union, then this union should involve both 64bit and 32bit version of user_regs_struct, which is provided by libc crate instead of the nix itself. However, the struct definition is bound to the compile target of libc crate, we cannot import a 32bit user_regs_struct in libc if we compiles to x86_64

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, the struct definition is bound to the compile target of libc crate, we cannot import a 32bit user_regs_struct in libc if we compiles to x86_64

Right, the actual type is decided at compile time, so we cannot use the 32-bit version unless we define it in Nix, which is not something Nix should do.

$ pwd
libc/src/unix/linux_like/linux/gnu

$ rg "struct user_regs_struct"
b64/x86_64/mod.rs
178:    pub struct user_regs_struct {

b32/x86/mod.rs
74:    pub struct user_regs_struct {

b64/loongarch64/mod.rs
193:    pub struct user_regs_struct {

b64/riscv64/mod.rs
196:    pub struct user_regs_struct {

b32/riscv32/mod.rs
200:    pub struct user_regs_struct {

b64/aarch64/mod.rs
146:    pub struct user_regs_struct {

As far as I know, we should check the target tracee bitness in advance, instead of checking after the syscall returns.

After figuring out the bitness, how do you use a 32-bit version structure in C if you know the tracee is a 32-bit process? Since in C, these types are also chosen at compile time with #ifdef macros.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In GDB, it is done without the user_regs_struct. The regs are fetched here, and is used here

In short, it provides a 64bit version buffer to receive the regs, and when storing it to internal structure, the tracee arch is checked and collect corresponding register values.

///
/// [ptrace(2)]: https://www.man7.org/linux/man-pages/man2/ptrace.2.html
#[cfg(all(
target_os = "linux",
Expand Down Expand Up @@ -342,6 +345,9 @@ pub fn getregs(pid: Pid) -> Result<user_regs_struct> {
}

/// Get a particular set of user registers, as with `ptrace(PTRACE_GETREGSET, ...)`
///
/// Currently, in x86_64 platform, if the tracer is 64bit and tracee is 32bit, this function
/// will return an error.
#[cfg(all(
target_os = "linux",
target_env = "gnu",
Expand All @@ -367,6 +373,9 @@ pub fn getregset<S: RegisterSet>(pid: Pid) -> Result<S::Regs> {
(&mut iov as *mut libc::iovec).cast(),
)?;
};
if iov.iov_len != mem::size_of::<S::Regs>() {
return Err(Errno::EIO);
}
Ok(unsafe { data.assume_init() })
}

Expand Down