pf: Fix for purecap kernel with subobject bounds #2282
Conversation
…c_qstats_v0 The struct sizes are used to generate IOCTL code numbers. In CHERI purecap, the struct sizes of pfioc_qstats_v1 and pfioc_qstats_v0 are identical, causing a compile error.
| @@ -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; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -1478,10 +1478,10 @@ pf_normalize_tcp(struct pfi_kkif *kif, struct mbuf *m, int ipoff, | |||
| (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); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I believe the following works:
ntohs(((uint16_t)th->th_off << 12) | ((uint16_t)th->th_x2 << 8) | th->th_flags)
| @@ -1598,7 +1598,7 @@ struct pf_pdesc { | |||
| #ifdef INET6 | |||
| struct icmp6_hdr icmp6; | |||
| #endif /* INET6 */ | |||
| char any[0]; | |||
There was a problem hiding this comment.
__subobject_use_container_bounds?
There was a problem hiding this comment.
Though really this is extremely pointless a member. One should just be casting a pointer to the union to a char * instead.
There was a problem hiding this comment.
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.
| @@ -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 */ | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
The issues that this PR fixes were found by fuzzing.
Tagging @YiChenChai.