Some improvements to comments in bpf_validate()

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

Some improvements to comments in bpf_validate()

Guy Harris
Here are some changes to better document what bpf_validate() in
sys/net/bpf_filter.c does (it checks memory accesses to some degree, and
unconditional branches *could* go backwards if the jump offset is large
enough to cause an overflow).
Index: bpf_filter.c
===================================================================
RCS file: /cvs/src/sys/net/bpf_filter.c,v
retrieving revision 1.15
diff -u -r1.15 bpf_filter.c
--- bpf_filter.c 28 Sep 2005 20:53:56 -0000 1.15
+++ bpf_filter.c 30 Nov 2005 10:21:33 -0000
@@ -475,9 +475,10 @@
 /*
  * Return true if the 'fcode' is a valid filter program.
  * The constraints are that each jump be forward and to a valid
- * code.  The code must terminate with either an accept or reject.
- * 'valid' is an array for use by the routine (it must be at least
- * 'len' bytes long).  
+ * code, that memory accesses are within valid ranges (to the
+ * extent that this can be checked statically; loads of packet
+ * data have to be, and are, also checked at run time), and that
+ * the code terminates with either an accept or reject.
  *
  * The kernel needs to be able to verify an application's filter code.
  * Otherwise, a bogus program could easily crash the system.
@@ -551,8 +552,20 @@
  break;
  case BPF_JMP:
  /*
- * Check that that jumps are forward, and within
- * the code block.
+ * Check that jumps are within the code block,
+ * and that unconditional branches don't go
+ * backwards as a result of an overflow.
+ * Unconditional branches have a 32-bit offset,
+ * so they could overflow; we check to make
+ * sure they don't.  Conditional branches have
+ * an 8-bit offset, and the from address is <=
+ * BPF_MAXINSNS, and we assume that BPF_MAXINSNS
+ * is sufficiently small that adding 255 to it
+ * won't overflow.
+ *
+ * We know that len is <= BPF_MAXINSNS, and we
+ * assume that BPF_MAXINSNS is < the maximum size
+ * of a u_int, so that i + 1 doesn't overflow.
  */
  from = i + 1;
  switch (BPF_OP(p->code)) {
@@ -578,7 +591,6 @@
  default:
  return 0;
  }
-
  }
  return BPF_CLASS(f[len - 1].code) == BPF_RET;
 }

Reply | Threaded
Open this post in threaded view
|

Re: Some improvements to comments in bpf_validate()

Otto Moerbeek
On Wed, 30 Nov 2005, Guy Harris wrote:

> Here are some changes to better document what bpf_validate() in
> sys/net/bpf_filter.c does (it checks memory accesses to some degree, and
> unconditional branches *could* go backwards if the jump offset is large
> enough to cause an overflow).

Thanks for sending a unified diff... ;-)

I think your description is correct, it reflects my reasoning when making
the more strict validation code some time ago.

Now since I wrote that code, I'm no good judge if the code itself
isn't clear enough. I'll let other developers decide if your comments
add value.

BTW: since this isn't about a bug, tech@ would be a better platform.

        -Otto


> Index: bpf_filter.c
> ===================================================================
> RCS file: /cvs/src/sys/net/bpf_filter.c,v
> retrieving revision 1.15
> diff -u -r1.15 bpf_filter.c
> --- bpf_filter.c 28 Sep 2005 20:53:56 -0000 1.15
> +++ bpf_filter.c 30 Nov 2005 10:21:33 -0000
> @@ -475,9 +475,10 @@
>  /*
>   * Return true if the 'fcode' is a valid filter program.
>   * The constraints are that each jump be forward and to a valid
> - * code.  The code must terminate with either an accept or reject.
> - * 'valid' is an array for use by the routine (it must be at least
> - * 'len' bytes long).  
> + * code, that memory accesses are within valid ranges (to the
> + * extent that this can be checked statically; loads of packet
> + * data have to be, and are, also checked at run time), and that
> + * the code terminates with either an accept or reject.
>   *
>   * The kernel needs to be able to verify an application's filter code.
>   * Otherwise, a bogus program could easily crash the system.
> @@ -551,8 +552,20 @@
>   break;
>   case BPF_JMP:
>   /*
> - * Check that that jumps are forward, and within
> - * the code block.
> + * Check that jumps are within the code block,
> + * and that unconditional branches don't go
> + * backwards as a result of an overflow.
> + * Unconditional branches have a 32-bit offset,
> + * so they could overflow; we check to make
> + * sure they don't.  Conditional branches have
> + * an 8-bit offset, and the from address is <=
> + * BPF_MAXINSNS, and we assume that BPF_MAXINSNS
> + * is sufficiently small that adding 255 to it
> + * won't overflow.
> + *
> + * We know that len is <= BPF_MAXINSNS, and we
> + * assume that BPF_MAXINSNS is < the maximum size
> + * of a u_int, so that i + 1 doesn't overflow.
>   */
>   from = i + 1;
>   switch (BPF_OP(p->code)) {
> @@ -578,7 +591,6 @@
>   default:
>   return 0;
>   }
> -
>   }
>   return BPF_CLASS(f[len - 1].code) == BPF_RET;
>  }