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

Fix freebsdNN compat for [u]intptr_t syscall arguments #2167

Merged
merged 4 commits into from
Aug 26, 2024

Conversation

bsdjhb
Copy link
Collaborator

@bsdjhb bsdjhb commented Aug 1, 2024

  • freebsd64_fcntl: Remove unnecessary cast
  • freebsd32/64: Map [u]intptr_t syscall arguments to [u]intXX_t
  • sysent: Regen

Copy link
Member

@brooksdavis brooksdavis left a comment

Choose a reason for hiding this comment

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

I'm now feeling uncomfortable about fcntl being intptr_t on 32-bit architectures with >31-bit userspace addresses. I think that may be making kernel addresses.

sys/compat/freebsd64/freebsd64_sysent.c Outdated Show resolved Hide resolved
@bsdjhb
Copy link
Collaborator Author

bsdjhb commented Aug 21, 2024

I'm now feeling uncomfortable about fcntl being intptr_t on 32-bit architectures with >31-bit userspace addresses. I think that may be making kernel addresses.

Yes, I wish it was a uintptr_t arg instead (I think)

@bsdjhb
Copy link
Collaborator Author

bsdjhb commented Aug 21, 2024

I'm now feeling uncomfortable about fcntl being intptr_t on 32-bit architectures with >31-bit userspace addresses. I think that may be making kernel addresses.

Yes, I wish it was a uintptr_t arg instead (I think)

freebsd32_fcntl() goes out of its way to use an unsigned cast to avoid sign extension for cases where it is either a pointer or flags field.

@bsdjhb
Copy link
Collaborator Author

bsdjhb commented Aug 21, 2024

I've changed this to add synthetic [u]intptrXX_t types that are used in place of [u]intptr_t. This avoids the false positive matches from isptrtype() in makesyscalls.lua and trims much of the noise from the diff.

@@ -14,6 +14,7 @@ abi_long="int32_t"
abi_u_long="uint32_t"
abi_semid_t="int32_t"
abi_size_t="uint32_t"
abi_intptr_t="intptr32_t"
Copy link
Member

Choose a reason for hiding this comment

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

I don't see what benefit the extra type gives over just saying int32_t?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It avoids all the extra gunk due to isptrtype() matching on non-pointer instances. I didn't see an easy way to fix it in makesyscalls.lua.

sys/sys/types.h Outdated
typedef int32_t intptr32_t;
typedef uint32_t uintptr32_t;
typedef int64_t intptr64_t;
typedef uint64_t uintptr64_t;
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit weird to have this on ILP32, and a bit weird for these to exist outside of compat code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I originally wanted to put these in <compat/freebsdNN/freebsdNN.h>, but many places include freebsdNN_proto.h without freebsdNN.h and so failed to compile. One of those places is the generated freebsdNN_sysent.c, so I tried to find a header that was already included by various places. Given that it is kernel only I could guard the typedefs with #ifdef COMPAT_FREEBSDnn.

sys/sys/types.h Outdated Show resolved Hide resolved
@bsdjhb bsdjhb merged commit f48970c into CTSRD-CHERI:dev Aug 26, 2024
29 checks passed
@bsdjhb bsdjhb deleted the compat_intptr_t branch August 26, 2024 18:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants