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.

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

pf: Fix for purecap kernel with subobject bounds #2282

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
+5 −4
Open

pf: Fix for purecap kernel with subobject bounds #2282

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8bc336f
pf: Disable CHERI subobject bounds in pf_addr
RoundofThree Dec 29, 2024
4d0013a
pf: Disable CHERI subobject bounds in pf_pdesc
RoundofThree Dec 29, 2024
f201118
pf: Add a padding to pfioc_qstats_v1 to have a different size to pfio…
RoundofThree Dec 29, 2024
29703da
pf: Avoid CHERI crashes due to an inocent OOB access of to-be-discard…
RoundofThree Dec 29, 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
4 changes: 2 additions & 2 deletions sys/netpfil/pf/pf_norm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1478,13 +1478,13 @@
(tcp_get_flags(th) & (TH_RES1|TH_RES2|TH_RES2)) != 0) {
u_int16_t ov, nv;

ov = *(u_int16_t *)(&th->th_ack + 1);
ov = *(u_int16_t *)(__unbounded_addressof(th->th_ack) + 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, maybe we should just not require subobject bounds on any members of struct tcphdr? There's really no good reason to narrow bounds to any individual members of this structure. I worry that other places will be doing this same trick to cope with the 12 bit flags field, so would rather fix it in the definition of struct tcphdr if possible.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, there's always good reason to narrow bounds, you never know when a pointer to a member might be being passed around.

Better would be to have (__)tcp_[gs]et_flags16 or something that just peeks/pokes each field and shifts everything around.

Copy link
Member

Choose a reason for hiding this comment

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

Well, you don't even need the set, just the get, as you just want to get the uint16_t that contains all the changed flag bits, in host order.

Copy link
Member

@jrtc27 jrtc27 Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I believe the following works:

ntohs(((uint16_t)th->th_off << 12) | ((uint16_t)th->th_x2 << 8) | th->th_flags)

https://godbolt.org/z/sT7abM6Kh

flags &= ~(TH_RES1 | TH_RES2 | TH_RES3);
tcp_set_flags(th, flags);
nv = *(u_int16_t *)(&th->th_ack + 1);
nv = *(u_int16_t *)(__unbounded_addressof(th->th_ack) + 1);

th->th_sum = pf_proto_cksum_fixup(m, th->th_sum, ov, nv, 0);
rewrite = 1;

Check warning on line 1487 in sys/netpfil/pf/pf_norm.c

View workflow job for this annotation

GitHub Actions / Style Checker 

Missing Signed-off-by: line
}

/* Remove urgent pointer, if TH_URG is not set */
Expand Down
Loading