Ship ubsan_minimal library in base?
How do people feel about shipping the minimal UBSan runtime library[1] in the base system? It takes very little to build (Makefile + a few ifdefs that both jca@ and I hacked together). The library is tiny and useful enough for finding UB in base. % ls -l /usr/lib/clang/13.0.0/lib/libclang_rt.ubsan_minimal* -r--r--r-- 1 root bin 51516 Feb 4 20:02 /usr/lib/clang/13.0.0/lib/libclang_rt.ubsan_minimal.a % cd /usr/src/games/battlestar % make obj && make clean; make LDFLAGS='-fsanitize=undefined -fsanitize-minimal-runtime' COPTS='-g -fsanitize=undefined -fsanitize-minimal-runtime -fno-wrapv' % ./obj/battlestar Version 4.2, fall 1984. First Adventure game written by His Lordship, the honorable Admiral D.W. Riggle ubsan: shift-out-of-bounds % egdb ./obj/battlestar (gdb) b __ubsan_handle_shift_out_of_bounds_minimal Breakpoint 1 at 0x23630: file /usr/src/gnu/lib/libclang_rt/ubsan_minimal/../../../llvm/compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp, line 104. (gdb) bt No stack. (gdb) r Starting program: /usr/obj/games/battlestar/battlestar Version 4.2, fall 1984. First Adventure game written by His Lordship, the honorable Admiral D.W. Riggle Breakpoint 1, __ubsan_handle_shift_out_of_bounds_minimal () at /usr/src/gnu/lib/libclang_rt/ubsan_minimal/../../../llvm/compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp:104 104 HANDLER(shift_out_of_bounds, "shift-out-of-bounds") (gdb) bt #0 __ubsan_handle_shift_out_of_bounds_minimal () at /usr/src/gnu/lib/libclang_rt/ubsan_minimal/../../../llvm/compiler-rt/lib/ubsan_minimal/ubsan_minimal_handlers.cpp:104 #1 0x07434ee9f654 in initialize (filename=) at /usr/src/games/battlestar/init.c:67 #2 0x07434ee8af06 in main (argc=1, argv=) at /usr/src/games/battlestar/battlestar.c:58 (gdb) up #1 0x0b1eddd2a654 in initialize (filename=) at /usr/src/games/battlestar/init.c:67 67 SetBit(location[p->room].objects, p->obj); (gdb) !grep SetBit extern.h #define SetBit(array, index)(array[index/BITS] |= (1 << (index % BITS))) Yup, the usual, shifting too far without 1U. Hence the patch: --- a/games/battlestar/extern.h +++ b/games/battlestar/extern.h @@ -39,9 +39,9 @@ #define OUTSIDE(position > 68 && position < 246 && position != 218) #define rnd(x) arc4random_uniform(x) #define max(a,b) ((a) < (b) ? (b) : (a)) -#define TestBit(array, index) (array[index/BITS] & (1 << (index % BITS))) -#define SetBit(array, index) (array[index/BITS] |= (1 << (index % BITS))) -#define ClearBit(array, index) (array[index/BITS] &= ~(1 << (index % BITS))) +#define TestBit(array, index) (array[index/BITS] & (1U << (index % BITS))) +#define SetBit(array, index) (array[index/BITS] |= (1U << (index % BITS))) +#define ClearBit(array, index) (array[index/BITS] &= ~(1U << (index % BITS))) /* * These macros yield words to use with objects (followed but not preceded * by spaces, or with no spaces if the expansion is the empty string). OK to fix battlestar while we are at it? [1] https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#minimal-runtime
Re: convert bgpd to stdint.h types
ok
On Sat, 5 Feb 2022, 01:08 Claudio Jeker, wrote:
> This is something I wanted to do for a while. Switch from u_intXY_t to
> uintXY_t from stdint.h. The diff is mostly mechanical and was done with
> sed -i 's/u_intX_t/uintX_t/g' but uint8_t changes the tab spacing and so
> I had a look over the code and reindented where it made sense.
> Using stdint.h types will mostly help portability.
>
> Sorry for the size of this diff.
> --
> :wq Claudio
>
> Index: bgpctl/bgpctl.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
> retrieving revision 1.274
> diff -u -p -r1.274 bgpctl.c
> --- bgpctl/bgpctl.c 4 Feb 2022 12:01:33 - 1.274
> +++ bgpctl/bgpctl.c 4 Feb 2022 14:31:26 -
> @@ -54,9 +54,9 @@ void show_mrt_dump(struct mrt_rib *, s
> voidnetwork_mrt_dump(struct mrt_rib *, struct mrt_peer *,
> void *);
> voidshow_mrt_state(struct mrt_bgp_state *, void *);
> voidshow_mrt_msg(struct mrt_bgp_msg *, void *);
> -const char *msg_type(u_int8_t);
> +const char *msg_type(uint8_t);
> voidnetwork_bulk(struct parse_result *);
> -int match_aspath(void *, u_int16_t, struct filter_as *);
> +int match_aspath(void *, uint16_t, struct filter_as *);
>
> struct imsgbuf *ibuf;
> struct mrt_parser show_mrt = { show_mrt_dump, show_mrt_state,
> show_mrt_msg };
> @@ -624,7 +624,7 @@ fmt_monotime(time_t t)
> }
>
> const char *
> -fmt_fib_flags(u_int16_t flags)
> +fmt_fib_flags(uint16_t flags)
> {
> static char buf[8];
>
> @@ -665,7 +665,7 @@ fmt_fib_flags(u_int16_t flags)
> }
>
> const char *
> -fmt_origin(u_int8_t origin, int sum)
> +fmt_origin(uint8_t origin, int sum)
> {
> switch (origin) {
> case ORIGIN_IGP:
> @@ -680,7 +680,7 @@ fmt_origin(u_int8_t origin, int sum)
> }
>
> const char *
> -fmt_flags(u_int8_t flags, int sum)
> +fmt_flags(uint8_t flags, int sum)
> {
> static char buf[80];
> char flagstr[5];
> @@ -723,7 +723,7 @@ fmt_flags(u_int8_t flags, int sum)
> }
>
> const char *
> -fmt_ovs(u_int8_t validation_state, int sum)
> +fmt_ovs(uint8_t validation_state, int sum)
> {
> switch (validation_state) {
> case ROA_INVALID:
> @@ -747,7 +747,7 @@ fmt_mem(long long num)
> }
>
> const char *
> -fmt_errstr(u_int8_t errcode, u_int8_t subcode)
> +fmt_errstr(uint8_t errcode, uint8_t subcode)
> {
> static char errbuf[256];
> const char *errstr = NULL;
> @@ -814,7 +814,7 @@ fmt_errstr(u_int8_t errcode, u_int8_t su
> }
>
> const char *
> -fmt_attr(u_int8_t type, int flags)
> +fmt_attr(uint8_t type, int flags)
> {
> #define CHECK_FLAGS(s, t, m) \
> if (((s) & ~(ATTR_DEFMASK | (m))) != (t)) pflags = 1
> @@ -909,7 +909,7 @@ fmt_attr(u_int8_t type, int flags)
> }
>
> const char *
> -fmt_community(u_int16_t a, u_int16_t v)
> +fmt_community(uint16_t a, uint16_t v)
> {
> static char buf[12];
>
> @@ -936,7 +936,7 @@ fmt_community(u_int16_t a, u_int16_t v)
> }
>
> const char *
> -fmt_large_community(u_int32_t d1, u_int32_t d2, u_int32_t d3)
> +fmt_large_community(uint32_t d1, uint32_t d2, uint32_t d3)
> {
> static char buf[33];
>
> @@ -945,14 +945,14 @@ fmt_large_community(u_int32_t d1, u_int3
> }
>
> const char *
> -fmt_ext_community(u_int8_t *data)
> +fmt_ext_community(uint8_t *data)
> {
> static char buf[32];
> - u_int64_t ext;
> + uint64_text;
> struct in_addr ip;
> - u_int32_t as4, u32;
> - u_int16_t as2, u16;
> - u_int8_ttype, subtype;
> + uint32_tas4, u32;
> + uint16_tas2, u16;
> + uint8_t type, subtype;
>
> type = data[0];
> subtype = data[1];
> @@ -1057,7 +1057,7 @@ network_bulk(struct parse_result *res)
> char *line = NULL;
> size_t linesize = 0;
> ssize_t linelen;
> - u_int8_t len;
> + uint8_t len;
> FILE *f;
>
> if ((f = fdopen(STDIN_FILENO, "r")) == NULL)
> @@ -1108,7 +1108,7 @@ show_mrt_dump_neighbors(struct mrt_rib *
> {
> struct mrt_peer_entry *p;
> struct in_addr ina;
> - u_int16_t i;
> + uint16_t i;
>
> ina.s_addr = htonl(mp->bgp_id);
> printf("view: %s BGP ID: %s Number of peers: %u\n\n",
> @@ -1132,7 +1132,7 @@ show_mrt_dump(struct mrt_rib *mr, struct
> struct ctl_show_rib_request *req = arg;
> struct mrt_rib_entry*mre;
> time_t now;
> - u_int16_ti, j;
> + uint16_t i, j;
>
> memset(&res, 0, sizeof(res));
> res.flags = req->flags;
> @@ -1214,7 +1214,7 @@ network_mrt_dump(struct mrt_rib *mr, str
> struct mrt_rib_entry*mre;
> struct ibuf *msg;
> time_t now;
Re: convert bgpd to stdint.h types
On Fri, Feb 04, 2022 at 07:20:21PM +0100, Theo Buehler wrote: > On Fri, Feb 04, 2022 at 03:59:34PM +0100, Claudio Jeker wrote: > > This is something I wanted to do for a while. Switch from u_intXY_t to > > uintXY_t from stdint.h. The diff is mostly mechanical and was done with > > sed -i 's/u_intX_t/uintX_t/g' but uint8_t changes the tab spacing and so > > I had a look over the code and reindented where it made sense. > > Using stdint.h types will mostly help portability. > > ok (the only object change on amd64 is in util.c - not sure why). > > I was wondering if u_char shouldn't be replaced as well. There are u_char, u_short and u_int in bgpd. I have not fully made up my mind about those. There will be a follow up diff for those. -- :wq Claudio
Re: convert bgpd to stdint.h types
On Fri, Feb 04, 2022 at 03:59:34PM +0100, Claudio Jeker wrote: > This is something I wanted to do for a while. Switch from u_intXY_t to > uintXY_t from stdint.h. The diff is mostly mechanical and was done with > sed -i 's/u_intX_t/uintX_t/g' but uint8_t changes the tab spacing and so > I had a look over the code and reindented where it made sense. > Using stdint.h types will mostly help portability. ok (the only object change on amd64 is in util.c - not sure why). I was wondering if u_char shouldn't be replaced as well.
Re: uvm_unmap_kill_entry(): unwire with map lock held
On 04/02/22(Fri) 03:39, Klemens Nanni wrote: > [...] > ... with the lock grabbed in uvm_map_teardown() that is, otherwise > the first call path can lock against itself (regress/misc/posixtestsuite > is a reproduce for this): > > vm_map_lock_read_ln+0x38 > uvm_fault_unwire+0x58 > uvm_unmap_kill_entry_withlock+0x68 > uvm_unmap_remove+0x2d4 > sys_munmap+0x11c > > which is obvious in hindsight. > > So grabbing the lock in uvm_map_teardown() instead avoids that while > still ensuring a locked map in the path missing a lock. This should be fine since this function should only be called when the last reference of a map is dropped. In other words the locking here is necessary to satisfy the assertions. I wonder if lock shouldn't be taken & released around uvm_map_teardown() which makes it easier to see that this is called after the last refcount decrement. > Index: uvm_map.c > === > RCS file: /cvs/src/sys/uvm/uvm_map.c,v > retrieving revision 1.282 > diff -u -p -r1.282 uvm_map.c > --- uvm_map.c 21 Dec 2021 22:21:32 - 1.282 > +++ uvm_map.c 4 Feb 2022 02:51:00 - > @@ -2734,6 +2751,7 @@ uvm_map_teardown(struct vm_map *map) > KERNEL_ASSERT_UNLOCKED(); > > KASSERT((map->flags & VM_MAP_INTRSAFE) == 0); > + vm_map_lock(map); > > /* Remove address selectors. */ > uvm_addr_destroy(map->uaddr_exe); >
Re: rpki-client: check certificate policies
On Fri, Feb 04, 2022 at 03:56:18PM +0100, Theo Buehler wrote:
> On Fri, Feb 04, 2022 at 12:03:41PM +0100, Claudio Jeker wrote:
> > On Fri, Feb 04, 2022 at 10:41:03AM +0100, Theo Buehler wrote:
> > > It was pointed out to Claudio that rpki-client does not enforce
> > > certificate policies.
> > >
> > > The diff below does that. It has two parts.
> > >
> > > In cert.c we check that the certificate policy extension matches the
> > > specification in RFC 6487, section 4.8.9, as amended by RFC 7318
> > > section 2. That's maybe a bit lengthy but completely straightforward.
> > > If you're curious what's in a CPS URI, it might be this:
> > > https://www.arin.net/resources/manage/rpki/cps.pdf
> > >
> > > The second bit is in parser.c and makes sure that the verifier builds a
> > > policy tree enforcing that the certification path uses a policy OID of
> > > id-cp-ipAddr-asNumber (see RFC 6484). This is a bit trickier since it
> > > involves X509_policy_check() under the hood. In short, we set up a
> > > policy containing that OID in the verify parameters and set verifier
> > > flags that ensure that the user set policy is enforced.
> > >
> > > If you will, the second part improves the validation. The verifier
> > > doesn't have a mechanism to enforce things like there's exactly one
> > > policy identifier, etc. That's what's done in cert.c
> > >
> > > This works for me. Please test.
> >
> > Looks good to me. OK claudio@
>
> Unfortunately, NID_ipAddr_asNumber is only available in LibreSSL 3.3 and
> later and didn't make it into OpenSSL 1.1 so I had to do some things a
> bit differently. I added a certpol_oid to x509_init_oid() and use it
> instead of NID_ipAddr_asNumber(). Since we have this global already,
> it's easier to use X509_VERIFY_PARAM_add0_policy(). It also forces one
> check to use OBJ_cmp() instead OBJ_obj2nid().
Sounds good to me. A comment below to that.
> > > + policy = sk_POLICYINFO_value(policies, 0);
> > > + assert(policy != NULL && policy->policyid != NULL);
> >
> > Should this be an assert() or a proper error check?
> > I guess sk_POLICYINFO_value() can not really fail because of the check
> > above. What about policy->policyid? I gess X509V3_EXT_d2i() sets this
> > correctly or it failed above.
>
> Yes. Both != NULL are guaranteed. You're right about sk_value(). Also
> your guess is correct: policy->policyid != NULL is guaranteed by the
> templated ASN.1:
>
> In POLICYINFO_seq_tt (lib/libcrypto/x509/x509_cpols.c:147), there is no
> ASN1_TFLG_OPTIONAL flag to indicate that policy->policyid can be NULL
> (whereas policy->qualifiers is optional). Therefore if policy != NULL
> and since X509V3_EXT_d2i didn't fail, it must have deserialized
> correctly, so policy->policyid != NULL.
>
> I left the assert as it is for now, but I can change it into an error
> check or drop it if you prefer.
I think the assert() is OK. I just want to make sure we don't abuse
assert() for proper error checking. There were some of those in the
codebase and in the end less assert() calls are preferred.
> > > + qualifier = sk_POLICYQUALINFO_value(qualifiers, 0);
> > > + assert(qualifier != NULL && qualifier->pqualid != NULL);
> >
> > Same as above.
>
> The answer is the same again, this time arguing with the flags in
> POLICYQUALINFO_seq_tt.
>
> > > + flags |= X509_V_FLAG_EXPLICIT_POLICY;
> > > + flags |= X509_V_FLAG_INHIBIT_MAP;
> >
> > I do not really understand the meaning of X509_V_FLAG_INHIBIT_MAP. Neither
> > the manpage nor the referenced RFC really help to explain what is
> > inhibited and if we really want that or not. It seems to be the more
> > strict option so I think it is the right thing to do but wow this is
> > complicated.
>
> It is super complicated indeed. Policy mappings are explained here:
> https://datatracker.ietf.org/doc/html/rfc5280#section-4.2.1.5
> It's another extension that allows translation between policy OIDs.
> I'm pretty sure we don't want to allow this.
Yes, I came to a similar conclusion after reading too much of rfc5280 for
my taste. Maybe something to reconsider once rfc8360 is needed (I still
hope it will just die and instead the original policy will be updated).
> Index: cert.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> retrieving revision 1.53
> diff -u -p -r1.53 cert.c
> --- cert.c20 Jan 2022 16:36:19 - 1.53
> +++ cert.c4 Feb 2022 14:32:50 -
> @@ -29,6 +29,7 @@
>
> #include
> #include
> +#include
>
> #include "extern.h"
>
> @@ -47,6 +48,7 @@ struct parse {
> const char *fn; /* currently-parsed file */
> };
>
> +extern ASN1_OBJECT *certpol_oid; /* id-cp-ipAddr-asNumber cert policy */
> extern ASN1_OBJECT *carepo_oid;/* 1.3.6.1.5.5.7.48.5 (caRepository) */
> extern ASN1_OBJECT *manifest_oid; /* 1.3.6.1.5.5.7.48.10 (rpkiManifest) */
> extern ASN1_OBJECT *notify_oid;/* 1.3.6.1.5.5.7.48.13 (rpkiNotify) */
> @@
Re: rpki-client: check certificate policies
On Fri, Feb 04, 2022 at 12:03:41PM +0100, Claudio Jeker wrote:
> On Fri, Feb 04, 2022 at 10:41:03AM +0100, Theo Buehler wrote:
> > It was pointed out to Claudio that rpki-client does not enforce
> > certificate policies.
> >
> > The diff below does that. It has two parts.
> >
> > In cert.c we check that the certificate policy extension matches the
> > specification in RFC 6487, section 4.8.9, as amended by RFC 7318
> > section 2. That's maybe a bit lengthy but completely straightforward.
> > If you're curious what's in a CPS URI, it might be this:
> > https://www.arin.net/resources/manage/rpki/cps.pdf
> >
> > The second bit is in parser.c and makes sure that the verifier builds a
> > policy tree enforcing that the certification path uses a policy OID of
> > id-cp-ipAddr-asNumber (see RFC 6484). This is a bit trickier since it
> > involves X509_policy_check() under the hood. In short, we set up a
> > policy containing that OID in the verify parameters and set verifier
> > flags that ensure that the user set policy is enforced.
> >
> > If you will, the second part improves the validation. The verifier
> > doesn't have a mechanism to enforce things like there's exactly one
> > policy identifier, etc. That's what's done in cert.c
> >
> > This works for me. Please test.
>
> Looks good to me. OK claudio@
Unfortunately, NID_ipAddr_asNumber is only available in LibreSSL 3.3 and
later and didn't make it into OpenSSL 1.1 so I had to do some things a
bit differently. I added a certpol_oid to x509_init_oid() and use it
instead of NID_ipAddr_asNumber(). Since we have this global already,
it's easier to use X509_VERIFY_PARAM_add0_policy(). It also forces one
check to use OBJ_cmp() instead OBJ_obj2nid().
> > + policy = sk_POLICYINFO_value(policies, 0);
> > + assert(policy != NULL && policy->policyid != NULL);
>
> Should this be an assert() or a proper error check?
> I guess sk_POLICYINFO_value() can not really fail because of the check
> above. What about policy->policyid? I gess X509V3_EXT_d2i() sets this
> correctly or it failed above.
Yes. Both != NULL are guaranteed. You're right about sk_value(). Also
your guess is correct: policy->policyid != NULL is guaranteed by the
templated ASN.1:
In POLICYINFO_seq_tt (lib/libcrypto/x509/x509_cpols.c:147), there is no
ASN1_TFLG_OPTIONAL flag to indicate that policy->policyid can be NULL
(whereas policy->qualifiers is optional). Therefore if policy != NULL
and since X509V3_EXT_d2i didn't fail, it must have deserialized
correctly, so policy->policyid != NULL.
I left the assert as it is for now, but I can change it into an error
check or drop it if you prefer.
> > + qualifier = sk_POLICYQUALINFO_value(qualifiers, 0);
> > + assert(qualifier != NULL && qualifier->pqualid != NULL);
>
> Same as above.
The answer is the same again, this time arguing with the flags in
POLICYQUALINFO_seq_tt.
> > + flags |= X509_V_FLAG_EXPLICIT_POLICY;
> > + flags |= X509_V_FLAG_INHIBIT_MAP;
>
> I do not really understand the meaning of X509_V_FLAG_INHIBIT_MAP. Neither
> the manpage nor the referenced RFC really help to explain what is
> inhibited and if we really want that or not. It seems to be the more
> strict option so I think it is the right thing to do but wow this is
> complicated.
It is super complicated indeed. Policy mappings are explained here:
https://datatracker.ietf.org/doc/html/rfc5280#section-4.2.1.5
It's another extension that allows translation between policy OIDs.
I'm pretty sure we don't want to allow this.
Index: cert.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
retrieving revision 1.53
diff -u -p -r1.53 cert.c
--- cert.c 20 Jan 2022 16:36:19 - 1.53
+++ cert.c 4 Feb 2022 14:32:50 -
@@ -29,6 +29,7 @@
#include
#include
+#include
#include "extern.h"
@@ -47,6 +48,7 @@ structparse {
const char *fn; /* currently-parsed file */
};
+extern ASN1_OBJECT *certpol_oid; /* id-cp-ipAddr-asNumber cert policy */
extern ASN1_OBJECT *carepo_oid;/* 1.3.6.1.5.5.7.48.5 (caRepository) */
extern ASN1_OBJECT *manifest_oid; /* 1.3.6.1.5.5.7.48.10 (rpkiManifest) */
extern ASN1_OBJECT *notify_oid;/* 1.3.6.1.5.5.7.48.13 (rpkiNotify) */
@@ -969,6 +971,78 @@ out:
return rc;
}
+static int
+certificate_policies(struct parse *p, X509_EXTENSION *ext)
+{
+ STACK_OF(POLICYINFO)*policies = NULL;
+ POLICYINFO *policy;
+ STACK_OF(POLICYQUALINFO)*qualifiers;
+ POLICYQUALINFO *qualifier;
+ int nid;
+ int rc = 0;
+
+ if (!X509_EXTENSION_get_critical(ext)) {
+ cryptowarnx("%s: RFC 6487 section 4.8.9: certificatePolicies: "
+ "extension not critical", p->fn);
+ goto out;
+ }
+
+ if ((policies = X509V3_EXT_d2i(ext)
Re: m_pullup alingment crash armv7 sparc64
On Fri, Feb 04, 2022 at 07:27:38PM +1000, David Gwynne wrote: > On Thu, Feb 03, 2022 at 11:39:50PM +0100, Alexander Bluhm wrote: > > On Thu, Feb 03, 2022 at 10:22:46PM +1000, David Gwynne wrote: > > > bpf should know better than this. it has all the information it needs to > > > align the payload properly, it just doesnt make enough of an effort. can > > > you try the diff below? > > > > Diff seems to work. regress/sbin/slaacd passes. > > I have started a full regress run on armv7 and sparc64. Regress passed. > tun(4) uses EMSGSIZE for this, but errno(2) says EMSGSIZE means > "Message too long". EINVAL sounds good. > > want me to do it now? As you move the code around, you can easily adopt the error code now. OK bluhm@
Re: always align data in m_pullup on all archs
On Fri, Feb 04, 2022 at 11:39:56AM +0100, Alexander Bluhm wrote: > On Fri, Feb 04, 2022 at 07:32:52PM +1000, David Gwynne wrote: > > as discussed in "m_pullup alingment crash armv7 sparc64", at worst it > > doesnt hurt to have m_pullup maintain the data alignment of payloads, > > and at best it will encourage aligned loads even if the arch allows > > unaligned accesses. aligned loads are faster than unaligned. > > > > ok? > > adj is unsigned int, so assigning a mtod(m0, unsigned long) looks > strange. Of course the higher bits are cut off anyway, but an > explicit & is clearer than a assingment with different types. > > Please use > > adj = mtod(m, unsigned long) & (sizeof(long) - 1); > adj = mtod(m0, unsigned long) & (sizeof(long) - 1); > > otherwise OK bluhm@ it's been pointed out to me that ALIGNBYTES is actually very conservative about alignment rather than optimistic. ie, it's at least sizeof(long) - 1, but can be bigger. i think i've been confusing it with ALIGNED_POINTER. > > > Index: uipc_mbuf.c > > === > > RCS file: /cvs/src/sys/kern/uipc_mbuf.c,v > > retrieving revision 1.280 > > diff -u -p -r1.280 uipc_mbuf.c > > --- uipc_mbuf.c 18 Jan 2022 12:38:21 - 1.280 > > +++ uipc_mbuf.c 4 Feb 2022 09:30:02 - > > @@ -945,9 +945,11 @@ m_pullup(struct mbuf *m0, int len) > > goto freem0; > > } > > > > - adj = mtod(m, unsigned long) & ALIGNBYTES; > > + adj = mtod(m, unsigned long); > > } else > > - adj = mtod(m0, unsigned long) & ALIGNBYTES; > > + adj = mtod(m0, unsigned long); > > + > > + adj &= sizeof(long) - 1; > > > > tail = head + M_SIZE(m0); > > head += adj;
Re: openbgpd vs illumos
Ok
On Fri, 4 Feb 2022, 21:19 Claudio Jeker, wrote:
> On illumos sun is defined by some header so better not use sun as a
> variable name. Rename variable to sa_un to reduce hacks in -portable.
>
> --
> :wq Claudio
>
> Index: bgpctl/bgpctl.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
> retrieving revision 1.273
> diff -u -p -r1.273 bgpctl.c
> --- bgpctl/bgpctl.c 9 Aug 2021 08:24:36 - 1.273
> +++ bgpctl/bgpctl.c 4 Feb 2022 11:10:31 -
> @@ -78,7 +78,7 @@ usage(void)
> int
> main(int argc, char *argv[])
> {
> - struct sockaddr_un sun;
> + struct sockaddr_un sa_un;
> int fd, n, done, ch, verbose = 0;
> struct imsg imsg;
> struct network_confignet;
> @@ -160,12 +160,12 @@ main(int argc, char *argv[])
> if ((fd = socket(AF_UNIX, SOCK_STREAM, 0)) == -1)
> err(1, "control_init: socket");
>
> - bzero(&sun, sizeof(sun));
> - sun.sun_family = AF_UNIX;
> - if (strlcpy(sun.sun_path, sockname, sizeof(sun.sun_path)) >=
> - sizeof(sun.sun_path))
> + bzero(&sa_un, sizeof(sa_un));
> + sa_un.sun_family = AF_UNIX;
> + if (strlcpy(sa_un.sun_path, sockname, sizeof(sa_un.sun_path)) >=
> + sizeof(sa_un.sun_path))
> errx(1, "socket name too long");
> - if (connect(fd, (struct sockaddr *)&sun, sizeof(sun)) == -1)
> + if (connect(fd, (struct sockaddr *)&sa_un, sizeof(sa_un)) == -1)
> err(1, "connect: %s", sockname);
>
> if (pledge("stdio", NULL) == -1)
> Index: bgpd/control.c
> ===
> RCS file: /cvs/src/usr.sbin/bgpd/control.c,v
> retrieving revision 1.105
> diff -u -p -r1.105 control.c
> --- bgpd/control.c 27 Apr 2021 15:34:18 - 1.105
> +++ bgpd/control.c 4 Feb 2022 11:07:25 -
> @@ -42,19 +42,19 @@ ssize_t imsg_read_nofd(struct imsgbuf
> int
> control_check(char *path)
> {
> - struct sockaddr_un sun;
> + struct sockaddr_un sa_un;
> int fd;
>
> - bzero(&sun, sizeof(sun));
> - sun.sun_family = AF_UNIX;
> - strlcpy(sun.sun_path, path, sizeof(sun.sun_path));
> + bzero(&sa_un, sizeof(sa_un));
> + sa_un.sun_family = AF_UNIX;
> + strlcpy(sa_un.sun_path, path, sizeof(sa_un.sun_path));
>
> if ((fd = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0)) == -1) {
> log_warn("%s: socket", __func__);
> return (-1);
> }
>
> - if (connect(fd, (struct sockaddr *)&sun, sizeof(sun)) == 0) {
> + if (connect(fd, (struct sockaddr *)&sa_un, sizeof(sa_un)) == 0) {
> log_warnx("control socket %s already in use", path);
> close(fd);
> return (-1);
> @@ -68,7 +68,7 @@ control_check(char *path)
> int
> control_init(int restricted, char *path)
> {
> - struct sockaddr_un sun;
> + struct sockaddr_un sa_un;
> int fd;
> mode_t old_umask, mode;
>
> @@ -78,10 +78,10 @@ control_init(int restricted, char *path)
> return (-1);
> }
>
> - bzero(&sun, sizeof(sun));
> - sun.sun_family = AF_UNIX;
> - if (strlcpy(sun.sun_path, path, sizeof(sun.sun_path)) >=
> - sizeof(sun.sun_path)) {
> + bzero(&sa_un, sizeof(sa_un));
> + sa_un.sun_family = AF_UNIX;
> + if (strlcpy(sa_un.sun_path, path, sizeof(sa_un.sun_path)) >=
> + sizeof(sa_un.sun_path)) {
> log_warn("control_init: socket name too long");
> close(fd);
> return (-1);
> @@ -102,7 +102,7 @@ control_init(int restricted, char *path)
> mode = S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP;
> }
>
> - if (bind(fd, (struct sockaddr *)&sun, sizeof(sun)) == -1) {
> + if (bind(fd, (struct sockaddr *)&sa_un, sizeof(sa_un)) == -1) {
> log_warn("control_init: bind: %s", path);
> close(fd);
> umask(old_umask);
> @@ -159,12 +159,12 @@ control_accept(int listenfd, int restric
> {
> int connfd;
> socklen_tlen;
> - struct sockaddr_un sun;
> + struct sockaddr_un sa_un;
> struct ctl_conn *ctl_conn;
>
> - len = sizeof(sun);
> + len = sizeof(sa_un);
> if ((connfd = accept4(listenfd,
> - (struct sockaddr *)&sun, &len,
> + (struct sockaddr *)&sa_un, &len,
> SOCK_NONBLOCK | SOCK_CLOEXEC)) == -1) {
> if (errno == ENFILE || errno == EMFILE) {
> pauseaccept = getmonotime();
>
>
Re: hardware checksum ix and ixl
On Fri, Jan 28, 2022 at 05:26:01PM +0100, Alexander Bluhm wrote:
> On Wed, Jan 26, 2022 at 11:05:51AM +0100, Claudio Jeker wrote:
> > On Wed, Jan 26, 2022 at 01:29:42AM +0100, Alexander Bluhm wrote:
> > > Hi,
> > >
> > > There were some problems with ix(4) and ixl(4) hardware checksumming
> > > for the output path on strict alignment architectures.
> > >
> > > I have merged jan@'s diffs and added some sanity checks and
> > > workarounds.
> > >
> > > - If the first mbuf is not aligned or not contigous, use m_copydata()
> > > to extract the IP, IPv6, TCP header.
> > > - If the header is in the first mbuf, use m_data for the fast path.
> > > - Add netstat counter for invalid header chains. This makes
> > > us aware when hardware checksumming fails.
> > > - Add netstat counter for header copies. This indicates that
> > > better storage allocation in the network stack is possible.
> > > It also allows to recognize alignment problems on non-strict
> > > architectures.
> > > - There is not risk of crashes on sparc64.
> > >
> > > Does this aproach make sense?
> >
> > I think it is overly complicated and too much data is copied around.
> > First of all there is no need to extract ipproto.
> > The code can just use the M_TCP_CSUM_OUT and M_UDP_CSUM_OUT flags (they
> > are not set for other protos).
> > Because of this only they ip_hlen needs to be accessed and this can be
> > done with m_getptr().
>
> A solution with m_getptr() is where we started. It has already
> been rejected. The problem is that there are 6 ways to implement
> this feature. Every option has its drawbacks and was rejected.
>
> Options are:
> 1. m_getptr() and access the struct. Alignment cannot be guaranteed.
> 2. m_getptr() and access the byte or word. Header fields should be
>accessed by structs.
> 3. Always m_copydata. Too much overhead.
> 4. Always use m_data. Kernel may crash or use invalid data.
> 5. Combination of m_data and m_copydata. Too complex.
> 6. Track the fields in mbuf header. Too fragile and memory overhead.
>
> In my measurements checksum offloading gave us 10% performance boost
> so I want this feature. Other drivers also have it.
>
> Could we get another idea or find a consensus which option to use?
after staring at this for a few hours my conclusion is option 1 actually
is the right approach, but the diff for ixl has a bug.
> > In the IP6 case even more can be skipped since ip_hlen is static for IPv6.
> >
> > In ixl(4) also the tcp header lenght needs to be extracted. Again the code
> > can be simplified because HW checksumming is only enabled if ip_hlen == 5
> > and so the offset of the th_off field is static (for both IPv4 and IPv6).
> > Again m_getptr can be used to just access the byte with th_off.
> >
> > Longterm in_proto_cksum_out() should probably help provide the th_off
> > field. I think enforcing ip_hlen == 5 for UDP and TCP is fine, who needs
> > IP options on UDP and TCP?
>
> Other diffs have been rejected as they make too many assumtions how
> the stack works.
my opinion is we can make these assumptions. the CSUM_OUT flags are
only set in very specific places where the stack itself is constructing
or checking properly aligned headers, and then the stack maintains
this alignment until it reaches the driver. this is where the first
of the bugs in the ixl diff comes in.
in the diff ixl_tx_setup_offload() is called after the dma mapping
occurs. this is implemented in this code:
static inline int
ixl_load_mbuf(bus_dma_tag_t dmat, bus_dmamap_t map, struct mbuf *m)
{
int error;
error = bus_dmamap_load_mbuf(dmat, map, m,
BUS_DMA_STREAMING | BUS_DMA_NOWAIT);
if (error != EFBIG)
return (error);
error = m_defrag(m, M_DONTWAIT);
if (error != 0)
return (error);
return (bus_dmamap_load_mbuf(dmat, map, m,
BUS_DMA_STREAMING | BUS_DMA_NOWAIT));
}
the problem is that when we get a heavily fragmented mbuf we call
m_defrag, which basically allocates a cluster and copies all the
data from the fragments into it. however, m_defrag does not maintain
the alignment of payload, meaning the ethernet header gets to start
on a 4 byte boundary cos that's what we get from the cluster pool,
and the IP and TCP payloads end up with a 2 byte misalignmnet.
my proposed solution to this is to set up the offload bits before
calling ixl_load_mbuf. i'm confident this is the bug that deraadt@ hit.
because of when the CSUM_OUT flags are set, i think m_getptr is fine for
pulling the mbuf apart. if it's not i want the code to blow up so it
gets fixed. that's why the code in my diff below lacks defenses.
the other bug is that the uint64_t holding the offload flags isn't
reset between being called for different mbufs. my solution to that is
to have ixl_tx_setup_offload() return the flags so the value in
ixl_start is overwritten every time.
lastly, ixl shouldn't (doesn't need to?) spelunk in the packet if
t
openbgpd vs illumos
On illumos sun is defined by some header so better not use sun as a
variable name. Rename variable to sa_un to reduce hacks in -portable.
--
:wq Claudio
Index: bgpctl/bgpctl.c
===
RCS file: /cvs/src/usr.sbin/bgpctl/bgpctl.c,v
retrieving revision 1.273
diff -u -p -r1.273 bgpctl.c
--- bgpctl/bgpctl.c 9 Aug 2021 08:24:36 - 1.273
+++ bgpctl/bgpctl.c 4 Feb 2022 11:10:31 -
@@ -78,7 +78,7 @@ usage(void)
int
main(int argc, char *argv[])
{
- struct sockaddr_un sun;
+ struct sockaddr_un sa_un;
int fd, n, done, ch, verbose = 0;
struct imsg imsg;
struct network_confignet;
@@ -160,12 +160,12 @@ main(int argc, char *argv[])
if ((fd = socket(AF_UNIX, SOCK_STREAM, 0)) == -1)
err(1, "control_init: socket");
- bzero(&sun, sizeof(sun));
- sun.sun_family = AF_UNIX;
- if (strlcpy(sun.sun_path, sockname, sizeof(sun.sun_path)) >=
- sizeof(sun.sun_path))
+ bzero(&sa_un, sizeof(sa_un));
+ sa_un.sun_family = AF_UNIX;
+ if (strlcpy(sa_un.sun_path, sockname, sizeof(sa_un.sun_path)) >=
+ sizeof(sa_un.sun_path))
errx(1, "socket name too long");
- if (connect(fd, (struct sockaddr *)&sun, sizeof(sun)) == -1)
+ if (connect(fd, (struct sockaddr *)&sa_un, sizeof(sa_un)) == -1)
err(1, "connect: %s", sockname);
if (pledge("stdio", NULL) == -1)
Index: bgpd/control.c
===
RCS file: /cvs/src/usr.sbin/bgpd/control.c,v
retrieving revision 1.105
diff -u -p -r1.105 control.c
--- bgpd/control.c 27 Apr 2021 15:34:18 - 1.105
+++ bgpd/control.c 4 Feb 2022 11:07:25 -
@@ -42,19 +42,19 @@ ssize_t imsg_read_nofd(struct imsgbuf
int
control_check(char *path)
{
- struct sockaddr_un sun;
+ struct sockaddr_un sa_un;
int fd;
- bzero(&sun, sizeof(sun));
- sun.sun_family = AF_UNIX;
- strlcpy(sun.sun_path, path, sizeof(sun.sun_path));
+ bzero(&sa_un, sizeof(sa_un));
+ sa_un.sun_family = AF_UNIX;
+ strlcpy(sa_un.sun_path, path, sizeof(sa_un.sun_path));
if ((fd = socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0)) == -1) {
log_warn("%s: socket", __func__);
return (-1);
}
- if (connect(fd, (struct sockaddr *)&sun, sizeof(sun)) == 0) {
+ if (connect(fd, (struct sockaddr *)&sa_un, sizeof(sa_un)) == 0) {
log_warnx("control socket %s already in use", path);
close(fd);
return (-1);
@@ -68,7 +68,7 @@ control_check(char *path)
int
control_init(int restricted, char *path)
{
- struct sockaddr_un sun;
+ struct sockaddr_un sa_un;
int fd;
mode_t old_umask, mode;
@@ -78,10 +78,10 @@ control_init(int restricted, char *path)
return (-1);
}
- bzero(&sun, sizeof(sun));
- sun.sun_family = AF_UNIX;
- if (strlcpy(sun.sun_path, path, sizeof(sun.sun_path)) >=
- sizeof(sun.sun_path)) {
+ bzero(&sa_un, sizeof(sa_un));
+ sa_un.sun_family = AF_UNIX;
+ if (strlcpy(sa_un.sun_path, path, sizeof(sa_un.sun_path)) >=
+ sizeof(sa_un.sun_path)) {
log_warn("control_init: socket name too long");
close(fd);
return (-1);
@@ -102,7 +102,7 @@ control_init(int restricted, char *path)
mode = S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP;
}
- if (bind(fd, (struct sockaddr *)&sun, sizeof(sun)) == -1) {
+ if (bind(fd, (struct sockaddr *)&sa_un, sizeof(sa_un)) == -1) {
log_warn("control_init: bind: %s", path);
close(fd);
umask(old_umask);
@@ -159,12 +159,12 @@ control_accept(int listenfd, int restric
{
int connfd;
socklen_tlen;
- struct sockaddr_un sun;
+ struct sockaddr_un sa_un;
struct ctl_conn *ctl_conn;
- len = sizeof(sun);
+ len = sizeof(sa_un);
if ((connfd = accept4(listenfd,
- (struct sockaddr *)&sun, &len,
+ (struct sockaddr *)&sa_un, &len,
SOCK_NONBLOCK | SOCK_CLOEXEC)) == -1) {
if (errno == ENFILE || errno == EMFILE) {
pauseaccept = getmonotime();
Re: rpki-client: check certificate policies
On Fri, Feb 04, 2022 at 10:41:03AM +0100, Theo Buehler wrote:
> It was pointed out to Claudio that rpki-client does not enforce
> certificate policies.
>
> The diff below does that. It has two parts.
>
> In cert.c we check that the certificate policy extension matches the
> specification in RFC 6487, section 4.8.9, as amended by RFC 7318
> section 2. That's maybe a bit lengthy but completely straightforward.
> If you're curious what's in a CPS URI, it might be this:
> https://www.arin.net/resources/manage/rpki/cps.pdf
>
> The second bit is in parser.c and makes sure that the verifier builds a
> policy tree enforcing that the certification path uses a policy OID of
> id-cp-ipAddr-asNumber (see RFC 6484). This is a bit trickier since it
> involves X509_policy_check() under the hood. In short, we set up a
> policy containing that OID in the verify parameters and set verifier
> flags that ensure that the user set policy is enforced.
>
> If you will, the second part improves the validation. The verifier
> doesn't have a mechanism to enforce things like there's exactly one
> policy identifier, etc. That's what's done in cert.c
>
> This works for me. Please test.
Looks good to me. OK claudio@
Few questions below.
> Index: cert.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> retrieving revision 1.53
> diff -u -p -r1.53 cert.c
> --- cert.c20 Jan 2022 16:36:19 - 1.53
> +++ cert.c4 Feb 2022 09:09:38 -
> @@ -29,6 +29,7 @@
>
> #include
> #include
> +#include
>
> #include "extern.h"
>
> @@ -969,6 +970,77 @@ out:
> return rc;
> }
>
> +static int
> +certificate_policies(struct parse *p, X509_EXTENSION *ext)
> +{
> + STACK_OF(POLICYINFO)*policies = NULL;
> + POLICYINFO *policy;
> + STACK_OF(POLICYQUALINFO)*qualifiers;
> + POLICYQUALINFO *qualifier;
> + int nid;
> + int rc = 0;
> +
> + if (!X509_EXTENSION_get_critical(ext)) {
> + cryptowarnx("%s: RFC 6487 section 4.8.9: certificatePolicies: "
> + "extension not critical", p->fn);
> + goto out;
> + }
> +
> + if ((policies = X509V3_EXT_d2i(ext)) == NULL) {
> + cryptowarnx("%s: RFC 6487 section 4.8.9: certificatePolicies: "
> + "failed extension parse", p->fn);
> + goto out;
> + }
> +
> + if (sk_POLICYINFO_num(policies) != 1) {
> + warnx("%s: RFC 6487 section 4.8.9: certificatePolicies: "
> + "want 1 policy, got %d", p->fn,
> + sk_POLICYINFO_num(policies));
> + goto out;
> + }
> +
> + policy = sk_POLICYINFO_value(policies, 0);
> + assert(policy != NULL && policy->policyid != NULL);
Should this be an assert() or a proper error check?
I guess sk_POLICYINFO_value() can not really fail because of the check
above. What about policy->policyid? I gess X509V3_EXT_d2i() sets this
correctly or it failed above.
> + if ((nid = OBJ_obj2nid(policy->policyid)) != NID_ipAddr_asNumber) {
> + warnx("%s: RFC 6487 section 4.8.9: certificatePolicies: "
> + "unexpected policy identifier %d (%s)", p->fn, nid,
> + OBJ_nid2sn(nid));
> + goto out;
> + }
> +
> + /* Policy qualifiers are optional. If they're absent, we're done. */
> + if ((qualifiers = policy->qualifiers) == NULL) {
> + rc = 1;
> + goto out;
> + }
> +
> + if (sk_POLICYQUALINFO_num(qualifiers) != 1) {
> + warnx("%s: RFC 7318 section 2: certificatePolicies: "
> + "want 1 policy qualifier, got %d", p->fn,
> + sk_POLICYQUALINFO_num(qualifiers));
> + goto out;
> + }
> +
> + qualifier = sk_POLICYQUALINFO_value(qualifiers, 0);
> + assert(qualifier != NULL && qualifier->pqualid != NULL);
Same as above.
> + if ((nid = OBJ_obj2nid(qualifier->pqualid)) != NID_id_qt_cps) {
> + warnx("%s: RFC 7318 section 2: certificatePolicies: "
> + "want CPS, got %d (%s)", p->fn, nid, OBJ_nid2sn(nid));
> + goto out;
> + }
> +
> + if (verbose > 1)
> + warnx("%s: CPS %.*s", p->fn, qualifier->d.cpsuri->length,
> + qualifier->d.cpsuri->data);
> +
> + rc = 1;
> + out:
> + sk_POLICYINFO_pop_free(policies, POLICYINFO_free);
> + return rc;
> +}
> +
> /*
> * Parse and partially validate an RPKI X509 certificate (either a trust
> * anchor or a certificate) as defined in RFC 6487.
> @@ -1024,6 +1096,9 @@ cert_parse_inner(const char *fn, const u
> case NID_sinfo_access:
> sia_present = 1;
> c = sbgp_sia(&p, ext);
> + break;
> + case NID_certificate_policies:
> + c = certifi
Re: always align data in m_pullup on all archs
On Fri, Feb 04, 2022 at 07:32:52PM +1000, David Gwynne wrote: > as discussed in "m_pullup alingment crash armv7 sparc64", at worst it > doesnt hurt to have m_pullup maintain the data alignment of payloads, > and at best it will encourage aligned loads even if the arch allows > unaligned accesses. aligned loads are faster than unaligned. > > ok? adj is unsigned int, so assigning a mtod(m0, unsigned long) looks strange. Of course the higher bits are cut off anyway, but an explicit & is clearer than a assingment with different types. Please use adj = mtod(m, unsigned long) & (sizeof(long) - 1); adj = mtod(m0, unsigned long) & (sizeof(long) - 1); otherwise OK bluhm@ > Index: uipc_mbuf.c > === > RCS file: /cvs/src/sys/kern/uipc_mbuf.c,v > retrieving revision 1.280 > diff -u -p -r1.280 uipc_mbuf.c > --- uipc_mbuf.c 18 Jan 2022 12:38:21 - 1.280 > +++ uipc_mbuf.c 4 Feb 2022 09:30:02 - > @@ -945,9 +945,11 @@ m_pullup(struct mbuf *m0, int len) > goto freem0; > } > > - adj = mtod(m, unsigned long) & ALIGNBYTES; > + adj = mtod(m, unsigned long); > } else > - adj = mtod(m0, unsigned long) & ALIGNBYTES; > + adj = mtod(m0, unsigned long); > + > + adj &= sizeof(long) - 1; > > tail = head + M_SIZE(m0); > head += adj;
rpki-client: check certificate policies
It was pointed out to Claudio that rpki-client does not enforce
certificate policies.
The diff below does that. It has two parts.
In cert.c we check that the certificate policy extension matches the
specification in RFC 6487, section 4.8.9, as amended by RFC 7318
section 2. That's maybe a bit lengthy but completely straightforward.
If you're curious what's in a CPS URI, it might be this:
https://www.arin.net/resources/manage/rpki/cps.pdf
The second bit is in parser.c and makes sure that the verifier builds a
policy tree enforcing that the certification path uses a policy OID of
id-cp-ipAddr-asNumber (see RFC 6484). This is a bit trickier since it
involves X509_policy_check() under the hood. In short, we set up a
policy containing that OID in the verify parameters and set verifier
flags that ensure that the user set policy is enforced.
If you will, the second part improves the validation. The verifier
doesn't have a mechanism to enforce things like there's exactly one
policy identifier, etc. That's what's done in cert.c
This works for me. Please test.
Index: cert.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
retrieving revision 1.53
diff -u -p -r1.53 cert.c
--- cert.c 20 Jan 2022 16:36:19 - 1.53
+++ cert.c 4 Feb 2022 09:09:38 -
@@ -29,6 +29,7 @@
#include
#include
+#include
#include "extern.h"
@@ -969,6 +970,77 @@ out:
return rc;
}
+static int
+certificate_policies(struct parse *p, X509_EXTENSION *ext)
+{
+ STACK_OF(POLICYINFO)*policies = NULL;
+ POLICYINFO *policy;
+ STACK_OF(POLICYQUALINFO)*qualifiers;
+ POLICYQUALINFO *qualifier;
+ int nid;
+ int rc = 0;
+
+ if (!X509_EXTENSION_get_critical(ext)) {
+ cryptowarnx("%s: RFC 6487 section 4.8.9: certificatePolicies: "
+ "extension not critical", p->fn);
+ goto out;
+ }
+
+ if ((policies = X509V3_EXT_d2i(ext)) == NULL) {
+ cryptowarnx("%s: RFC 6487 section 4.8.9: certificatePolicies: "
+ "failed extension parse", p->fn);
+ goto out;
+ }
+
+ if (sk_POLICYINFO_num(policies) != 1) {
+ warnx("%s: RFC 6487 section 4.8.9: certificatePolicies: "
+ "want 1 policy, got %d", p->fn,
+ sk_POLICYINFO_num(policies));
+ goto out;
+ }
+
+ policy = sk_POLICYINFO_value(policies, 0);
+ assert(policy != NULL && policy->policyid != NULL);
+
+ if ((nid = OBJ_obj2nid(policy->policyid)) != NID_ipAddr_asNumber) {
+ warnx("%s: RFC 6487 section 4.8.9: certificatePolicies: "
+ "unexpected policy identifier %d (%s)", p->fn, nid,
+ OBJ_nid2sn(nid));
+ goto out;
+ }
+
+ /* Policy qualifiers are optional. If they're absent, we're done. */
+ if ((qualifiers = policy->qualifiers) == NULL) {
+ rc = 1;
+ goto out;
+ }
+
+ if (sk_POLICYQUALINFO_num(qualifiers) != 1) {
+ warnx("%s: RFC 7318 section 2: certificatePolicies: "
+ "want 1 policy qualifier, got %d", p->fn,
+ sk_POLICYQUALINFO_num(qualifiers));
+ goto out;
+ }
+
+ qualifier = sk_POLICYQUALINFO_value(qualifiers, 0);
+ assert(qualifier != NULL && qualifier->pqualid != NULL);
+
+ if ((nid = OBJ_obj2nid(qualifier->pqualid)) != NID_id_qt_cps) {
+ warnx("%s: RFC 7318 section 2: certificatePolicies: "
+ "want CPS, got %d (%s)", p->fn, nid, OBJ_nid2sn(nid));
+ goto out;
+ }
+
+ if (verbose > 1)
+ warnx("%s: CPS %.*s", p->fn, qualifier->d.cpsuri->length,
+ qualifier->d.cpsuri->data);
+
+ rc = 1;
+ out:
+ sk_POLICYINFO_pop_free(policies, POLICYINFO_free);
+ return rc;
+}
+
/*
* Parse and partially validate an RPKI X509 certificate (either a trust
* anchor or a certificate) as defined in RFC 6487.
@@ -1024,6 +1096,9 @@ cert_parse_inner(const char *fn, const u
case NID_sinfo_access:
sia_present = 1;
c = sbgp_sia(&p, ext);
+ break;
+ case NID_certificate_policies:
+ c = certificate_policies(&p, ext);
break;
case NID_crl_distribution_points:
/* ignored here, handled later */
Index: parser.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
retrieving revision 1.58
diff -u -p -r1.58 parser.c
--- parser.c28 Jan 2022 15:30:23 - 1.58
+++ parser.c4 Feb 2022 00:15:02 -
@@ -43,9 +43,10 @@ stati
always align data in m_pullup on all archs
as discussed in "m_pullup alingment crash armv7 sparc64", at worst it doesnt hurt to have m_pullup maintain the data alignment of payloads, and at best it will encourage aligned loads even if the arch allows unaligned accesses. aligned loads are faster than unaligned. ok? Index: uipc_mbuf.c === RCS file: /cvs/src/sys/kern/uipc_mbuf.c,v retrieving revision 1.280 diff -u -p -r1.280 uipc_mbuf.c --- uipc_mbuf.c 18 Jan 2022 12:38:21 - 1.280 +++ uipc_mbuf.c 4 Feb 2022 09:30:02 - @@ -945,9 +945,11 @@ m_pullup(struct mbuf *m0, int len) goto freem0; } - adj = mtod(m, unsigned long) & ALIGNBYTES; + adj = mtod(m, unsigned long); } else - adj = mtod(m0, unsigned long) & ALIGNBYTES; + adj = mtod(m0, unsigned long); + + adj &= sizeof(long) - 1; tail = head + M_SIZE(m0); head += adj;
Re: m_pullup alingment crash armv7 sparc64
On Thu, Feb 03, 2022 at 11:39:50PM +0100, Alexander Bluhm wrote:
> On Thu, Feb 03, 2022 at 10:22:46PM +1000, David Gwynne wrote:
> > bpf should know better than this. it has all the information it needs to
> > align the payload properly, it just doesnt make enough of an effort. can
> > you try the diff below?
>
> Diff seems to work. regress/sbin/slaacd passes.
> I have started a full regress run on armv7 and sparc64.
cool.
>
> > + if (len < hlen)
> > + return (EPERM);
>
> This is not a permission problem. Maybe EINVAL.
this is preveserving the existing semantics of this chunk:
@@ -232,10 +247,6 @@ bpf_movein(struct uio *uio, struct bpf_d
goto bad;
}
- if (m->m_len < hlen) {
- error = EPERM;
- goto bad;
- }
/*
* Make room for link header, and copy it to sockaddr
*/
i was going to follow this diff up with another one to tweak it.
tun(4) uses EMSGSIZE for this, but errno(2) says EMSGSIZE means
"Message too long". EINVAL sounds good.
want me to do it now?
>
> > + /*
> > + * Get the length of the payload so we can align it properly.
> > +*/
>
> Whitespace misaligned.
yep.
> > + if (mlen > MAXMCLBYTES)
> > + return (EIO);
>
> Should we use EMSGSIZE like in bpfwrite() if the data does not fit.
yes. want me to do that now too?
>
> > + mlen = max(max_linkhdr, hlen) + roundup(alen, sizeof(long));
> ...
> > + m_align(m, alen); /* Align the payload. */
>
> sizeof(long) seems the correct alignment for mbuf. In rev 1.237
> of m_pullup() you introduced ALIGNBYTES. This is architecture
> dependent and the only place in the network stack where it is used.
> Should it also be sizeof(long) there?
i probably did that when i was adding ALIGNED_POINTER checks to ethernet
tunnel drivers.
my initial thought is it doesnt save m_pullup any work, so it's
probably better to use sizeof(long) so there's a better chance that
accesses to words in a pulled up payload are aligned. even if an
arch doesnt fault on unaligned access it still goes faster with
aligned accesses. if m_pullup is going to move data around it may as
well move it to the best place possible.
i'll spit a diff out for that next.
Index: bpf.c
===
RCS file: /cvs/src/sys/net/bpf.c,v
retrieving revision 1.210
diff -u -p -r1.210 bpf.c
--- bpf.c 16 Jan 2022 06:27:14 - 1.210
+++ bpf.c 4 Feb 2022 09:25:40 -
@@ -144,7 +144,7 @@ bpf_movein(struct uio *uio, struct bpf_d
struct mbuf *m;
struct m_tag *mtag;
int error;
- u_int hlen;
+ u_int hlen, alen, mlen;
u_int len;
u_int linktype;
u_int slen;
@@ -198,23 +198,38 @@ bpf_movein(struct uio *uio, struct bpf_d
return (EIO);
}
- if (uio->uio_resid > MAXMCLBYTES)
- return (EIO);
len = uio->uio_resid;
+ if (len < hlen)
+ return (EPERM);
- MGETHDR(m, M_WAIT, MT_DATA);
- m->m_pkthdr.ph_ifidx = 0;
- m->m_pkthdr.len = len - hlen;
+ /*
+* Get the length of the payload so we can align it properly.
+*/
+ alen = len - hlen;
+
+ /*
+* Allocate enough space for headers and the aligned payload.
+*/
+ mlen = max(max_linkhdr, hlen) + roundup(alen, sizeof(long));
+
+ if (mlen > MAXMCLBYTES)
+ return (EIO);
- if (len > MHLEN) {
- MCLGETL(m, M_WAIT, len);
+ MGETHDR(m, M_WAIT, MT_DATA);
+ if (mlen > MHLEN) {
+ MCLGETL(m, M_WAIT, mlen);
if ((m->m_flags & M_EXT) == 0) {
error = ENOBUFS;
goto bad;
}
}
+
+ m_align(m, alen); /* Align the payload. */
+ m->m_data -= hlen;
+
+ m->m_pkthdr.ph_ifidx = 0;
+ m->m_pkthdr.len = len;
m->m_len = len;
- *mp = m;
error = uiomove(mtod(m, caddr_t), len, uio);
if (error)
@@ -232,10 +247,6 @@ bpf_movein(struct uio *uio, struct bpf_d
goto bad;
}
- if (m->m_len < hlen) {
- error = EPERM;
- goto bad;
- }
/*
* Make room for link header, and copy it to sockaddr
*/
@@ -249,8 +260,10 @@ bpf_movein(struct uio *uio, struct bpf_d
sockp->sa_family = ntohl(af);
} else
memcpy(sockp->sa_data, m->m_data, hlen);
+
+ m->m_pkthdr.len -= hlen;
m->m_len -= hlen;
- m->m_data += hlen; /* XXX */
+ m->m_data += hlen;
}
/*
@@ -260,6 +273,7 @@ bpf_movein(struct uio *uio, struct bpf_d
*(u_int *)(mtag + 1) = linktype;
m_tag_prepend(m, mtag);
+ *mp = m;
return (0);
bad:
m_freem(m);
