Re: CVS commit: src/sys

[Roy Marples <roy%marples.name@localhost>]

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