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

Show file tree
Hide file tree
Changes from all commits
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
3 changes: 2 additions & 1 deletion sys/net/pfvar.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1598,7 +1598,7 @@ struct pf_pdesc {
#ifdef INET6
struct icmp6_hdr icmp6;
#endif /* INET6 */
char any[0];
Copy link
Member

Choose a reason for hiding this comment

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

__subobject_use_container_bounds?

Copy link
Member

Choose a reason for hiding this comment

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

Though really this is extremely pointless a member. One should just be casting a pointer to the union to a char * instead.

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.

Given there are only the following four uses in sys that I can find:

origin/dev:sys/netpfil/pf/pf.c:3667:     m_copyback(m, off, hdrlen, pd->hdr.any);
origin/dev:sys/netpfil/pf/pf.c:5240:         m_copyback(m, off, hdrlen, pd->hdr.any);
origin/dev:sys/netpfil/pf/pf.c:5290:     m_copyback(m, off, hdrlen, pd->hdr.any);
origin/dev:sys/netpfil/pf/pf.c:5524:         m_copyback(m, off, hdrlen, pd->hdr.any);

I'd rather see the code be less dodgy and cast instead. This is something that can be upstreamed.

char any[0] __no_subobject_bounds;
} hdr;

struct pf_krule *nat_rule; /* nat/rdr rule applied to packet */
Expand Down Expand Up @@ -1918,6 +1918,7 @@ struct pfioc_qstats_v1 {
* written entirely in terms of the v0 or v1 type.
*/
u_int32_t version; /* Requested version of stats struct */
void *pad;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would not prefer this method of dealing with this issue. Just #ifdef out the v0 constant in the various switch statements instead for purecap.

Copy link
Member

Choose a reason for hiding this comment

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

Depending on how you do freebsd64 (or freebsd32) you may still need the native ABI to have unique ioctl numbers for these. Though it looks like there's no compat support for these currently. I know @kprovost discovered this encoding collision a while ago but don't recall what he thought the best fix was.

};

/* Latest version of struct pfioc_qstats_vX */
Expand Down
2 changes: 1 addition & 1 deletion sys/netpfil/pf/pf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ struct pf_addr {
u_int8_t addr8[16];
u_int16_t addr16[8];
u_int32_t addr32[4];
}; /* 128-bit address */
} __no_subobject_bounds; /* 128-bit address */
Copy link
Member

Choose a reason for hiding this comment

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

There's no "why" in the commit message for this change. Presumably pointers to the union members are being cast to others, or turned back into a struct pf_addr? But I don't know the details to know whether this is the right thing to be doing (likely it points at crappy code elsewhere we should fix upstream).

Copy link
Member

Choose a reason for hiding this comment

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

In particular I know @kprovost discovered some issues upstream where the code was dodgy by trying out a purecap kernel a while back, but don't know if those all got polished and committed. I'd rather not paper over bad code with opt-out annotations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Some of the issues I found did get pushed upstream, but some I just papered over as well. I need to dig up that tree and see what did and did not make it in.

};

#define PFI_AFLAG_NETWORK 0x01
Expand Down
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