v5: make apf_run() return more helpful value on firmware integration bugs

Test: TreeHugger, manually
Signed-off-by: Maciej Żenczykowski <maze@google.com>
Change-Id: I4a904dac4a9bf21e3670af029d65587d44548c54

3 files changed, 40 insertions, 18 deletions

diff --git a/v5/apf_interpreter.c b/v5/apf_interpreter.c
index 7286794..1ec14c6 100644
--- a/v5/apf_interpreter.c
+++ b/v5/apf_interpreter.c
@@ -582,7 +582,7 @@ extern void APF_TRACE_HOOK(u32 pc, const u32* regs, const u8* program,
 /* Return code indicating "packet" should accepted. */
 #define PASS 1
 /* Return code indicating "packet" should be accepted (and something unexpected happened). */
-#define EXCEPTION PASS
+#define EXCEPTION 2
 /* Return code indicating "packet" should be dropped. */
 #define DROP 0
 /* Verify an internal condition and accept packet if it fails. */
@@ -593,7 +593,7 @@ extern void APF_TRACE_HOOK(u32 pc, const u32* regs, const u8* program,
 #define ENFORCE_UNSIGNED(c) ((c)==(u32)(c))
 
 u32 apf_version(void) {
-    return 20240316;
+    return 20240317;
 }
 
 typedef struct {
@@ -657,12 +657,16 @@ static int do_apf_run(apf_context* ctx) {
 /* Accept packet if not within data bounds */
 #define ASSERT_IN_DATA_BOUNDS(p, size) ASSERT_RETURN(IN_DATA_BOUNDS(p, size))
 
+    /* APFv6 requires at least 5 u32 counters at the end of ram, this makes counter[-5]++ valid */
+    /* This cannot wrap due to previous check, that enforced program_len & ram_len < 2GiB. */
+    if (ctx->program_len + 20 > ctx->ram_len) return EXCEPTION;
+
     /* Counters start at end of RAM and count *backwards* so this array takes negative integers. */
     u32 *counter = (u32*)(ctx->program + ctx->ram_len);
 
-    ASSERT_IN_PACKET_BOUNDS(ETH_HLEN);
-    /* Only populate if IP version is IPv4. */
-    if ((ctx->packet[ETH_HLEN] & 0xf0) == 0x40) {
+    /* Only populate if packet long enough, and IP version is IPv4. */
+    /* Note: this doesn't actually check the ethertype... */
+    if ((ctx->packet_len >= ETH_HLEN + IPV4_HLEN) && ((ctx->packet[ETH_HLEN] & 0xf0) == 0x40)) {
         ctx->mem.named.ipv4_header_size = (ctx->packet[ETH_HLEN] & 15) * 4;
     }
     /* Count of instructions remaining to execute. This is done to ensure an */
@@ -1054,9 +1058,9 @@ int apf_run(void* ctx, u32* const program, const u32 program_len,
     /* We also don't want garbage like program_len == 0xFFFFFFFF */
     if ((program_len | ram_len) >> 31) return EXCEPTION;
 
-    /* APFv6 requires at least 5 u32 counters at the end of ram, this makes counter[-5]++ valid */
-    /* This cannot wrap due to previous check. */
-    if (program_len + 20 > ram_len) return EXCEPTION;
+    /* Any valid ethernet packet should be at least ETH_HLEN long... */
+    if (!packet) return EXCEPTION;
+    if (packet_len < ETH_HLEN) return EXCEPTION;
 
     apf_context apf_ctx = { 0 };
     apf_ctx.caller_ctx = ctx;
@@ -1075,5 +1079,7 @@ int apf_run(void* ctx, u32* const program, const u32 program_len,
 
     int ret = do_apf_run(&apf_ctx);
     if (apf_ctx.tx_buf) do_discard_buffer(&apf_ctx);
+    /* Convert any exceptions internal to the program to just normal 'PASS' */
+    if (ret >= EXCEPTION) ret = PASS;
     return ret;
 }
diff --git a/v5/apf_interpreter.h b/v5/apf_interpreter.h
index fe3e7af..10821bf 100644
--- a/v5/apf_interpreter.h
+++ b/v5/apf_interpreter.h
@@ -127,8 +127,18 @@ int apf_transmit_buffer(void* ctx, uint8_t* ptr, uint32_t len, uint8_t dscp);
  * @param filter_age_16384ths - the number of 1/16384 seconds since the filter
  *                     was programmed.
  *
- * @return non-zero if packet should be passed,
- *         zero if packet should be dropped.
+ * @return zero if packet should be dropped,
+ *         non-zero if packet should be passed, specifically:
+ *         - 1 on normal apf program execution,
+ *         - 2 on exceptional circumstances (caused by bad firmware integration),
+ *           these include:
+ *             - program pointer which is not aligned to 4 bytes
+ *             - ram_len which is not a multiple of 4 bytes
+ *             - excessively large (>= 2GiB) program_len or ram_len
+ *             - packet pointer which is a null pointer
+ *             - packet_len shorter than ETH_HLEN (14)
+ *           As such, you may want to log a firmware exception if 2 is ever returned...
+ *
  *
  * NOTE: How to calculate filter_age_16384ths:
  *
diff --git a/v5/apf_interpreter_source.c b/v5/apf_interpreter_source.c
index 18edcf4..a515964 100644
--- a/v5/apf_interpreter_source.c
+++ b/v5/apf_interpreter_source.c
@@ -52,7 +52,7 @@ extern void APF_TRACE_HOOK(u32 pc, const u32* regs, const u8* program,
 // Return code indicating "packet" should accepted.
 #define PASS 1
 // Return code indicating "packet" should be accepted (and something unexpected happened).
-#define EXCEPTION PASS
+#define EXCEPTION 2
 // Return code indicating "packet" should be dropped.
 #define DROP 0
 // Verify an internal condition and accept packet if it fails.
@@ -63,7 +63,7 @@ extern void APF_TRACE_HOOK(u32 pc, const u32* regs, const u8* program,
 #define ENFORCE_UNSIGNED(c) ((c)==(u32)(c))
 
 u32 apf_version(void) {
-    return 20240316;
+    return 20240317;
 }
 
 typedef struct {
@@ -127,12 +127,16 @@ static int do_apf_run(apf_context* ctx) {
 // Accept packet if not within data bounds
 #define ASSERT_IN_DATA_BOUNDS(p, size) ASSERT_RETURN(IN_DATA_BOUNDS(p, size))
 
+    // APFv6 requires at least 5 u32 counters at the end of ram, this makes counter[-5]++ valid
+    // This cannot wrap due to previous check, that enforced program_len & ram_len < 2GiB.
+    if (ctx->program_len + 20 > ctx->ram_len) return EXCEPTION;
+
     // Counters start at end of RAM and count *backwards* so this array takes negative integers.
     u32 *counter = (u32*)(ctx->program + ctx->ram_len);
 
-    ASSERT_IN_PACKET_BOUNDS(ETH_HLEN);
-    // Only populate if IP version is IPv4.
-    if ((ctx->packet[ETH_HLEN] & 0xf0) == 0x40) {
+    // Only populate if packet long enough, and IP version is IPv4.
+    // Note: this doesn't actually check the ethertype...
+    if ((ctx->packet_len >= ETH_HLEN + IPV4_HLEN) && ((ctx->packet[ETH_HLEN] & 0xf0) == 0x40)) {
         ctx->mem.named.ipv4_header_size = (ctx->packet[ETH_HLEN] & 15) * 4;
     }
     // Count of instructions remaining to execute. This is done to ensure an
@@ -524,9 +528,9 @@ int apf_run(void* ctx, u32* const program, const u32 program_len,
     // We also don't want garbage like program_len == 0xFFFFFFFF
     if ((program_len | ram_len) >> 31) return EXCEPTION;
 
-    // APFv6 requires at least 5 u32 counters at the end of ram, this makes counter[-5]++ valid
-    // This cannot wrap due to previous check.
-    if (program_len + 20 > ram_len) return EXCEPTION;
+    // Any valid ethernet packet should be at least ETH_HLEN long...
+    if (!packet) return EXCEPTION;
+    if (packet_len < ETH_HLEN) return EXCEPTION;
 
     apf_context apf_ctx = { 0 };
     apf_ctx.caller_ctx = ctx;
@@ -545,5 +549,7 @@ int apf_run(void* ctx, u32* const program, const u32 program_len,
 
     int ret = do_apf_run(&apf_ctx);
     if (apf_ctx.tx_buf) do_discard_buffer(&apf_ctx);
+    // Convert any exceptions internal to the program to just normal 'PASS'
+    if (ret >= EXCEPTION) ret = PASS;
     return ret;
 }