Hi David
On 03/02/2021 21:45, David Young wrote:
>
> This change looks a little hasty to me.
>
> It looks to me like some of these structs were __packed so that
> they could be read/written directly from/to any offset in a packet
> chain using mtod(), which does not pay any mind to the alignment
> of `*t`:
>
> #define mtod(m, t) ((t)((m)->m_data))
>
> I see gre_h is accessed in that way, just for one example.
Looking at the other places where this is handled, does this patch to gre_h
address your concerns?
I tested this on aarch64 which does not define __NO_STRICT_ALIGNMENT and it
passes the KASSERT.
Roy
diff -r e3c82b1d9c2e sys/net/if_gre.c
--- a/sys/net/if_gre.c Mon Feb 08 01:00:49 2021 +0000
+++ b/sys/net/if_gre.c Tue Feb 09 06:55:44 2021 +0000
@@ -395,10 +395,26 @@
sc->sc_error_ev.ev_count++;
return;
}
- if (m->m_len < sizeof(*gh) && (m = m_pullup(m, sizeof(*gh))) == NULL) {
- GRE_DPRINTF(sc, "m_pullup failed\n");
- sc->sc_pullup_ev.ev_count++;
- return;
+
+ /* If the GRE header is not aligned, slurp it up into a new
+ * mbuf with space for link headers, in the event we forward
+ * it. Otherwise, if it is aligned, make sure the entire
+ * base GRE header is in the first mbuf of the chain.
+ */
+ if (GRE_HDR_ALIGNED_P(mtod(m, void *)) == 0) {
+ if ((m = m_copyup(m, sizeof(struct gre_h),
+ (max_linkhdr + 3) & ~3)) == NULL) {
+ /* XXXJRT new stat, please */
+ GRE_DPRINTF(sc, "m_copyup failed\n");
+ sc->sc_pullup_ev.ev_count++;
+ return;
+ }
+ } else if (__predict_false(m->m_len < sizeof(struct gre_h))) {
+ if ((m = m_pullup(m, sizeof(struct gre_h))) == NULL) {
+ GRE_DPRINTF(sc, "m_pullup failed\n");
+ sc->sc_pullup_ev.ev_count++;
+ return;
+ }
}
gh = mtod(m, const struct gre_h *);
@@ -940,7 +956,6 @@
#endif
M_PREPEND(m, sizeof(*gh), M_DONTWAIT);
-
if (m == NULL) {
IF_DROP(&ifp->if_snd);
error = ENOBUFS;
@@ -948,6 +963,7 @@
}
gh = mtod(m, struct gre_h *);
+ KASSERT(GRE_HDR_ALIGNED_P(gh));
gh->flags = 0;
gh->ptype = etype;
/* XXX Need to handle IP ToS. Look at how I handle IP TTL. */
diff -r e3c82b1d9c2e sys/net/if_gre.h
--- a/sys/net/if_gre.h Mon Feb 08 01:00:49 2021 +0000
+++ b/sys/net/if_gre.h Tue Feb 09 06:55:44 2021 +0000
@@ -131,6 +131,11 @@
Present if (rt_pres == 1)
*/
};
+#ifdef __NO_STRICT_ALIGNMENT
+#define GRE_HDR_ALIGNED_P(gh) 1
+#else
+#define GRE_HDR_ALIGNED_P(gh) ((((vaddr_t) (gh)) & 3) == 0)
+#endif
#ifdef __CTASSERT
__CTASSERT(sizeof(struct gre_h) == 4);
#endif
