Re: [ovs-dev] [PATCH v5] pinctrl: Check requested IP in DHCPREQUEST messages

2018-12-11 Thread Ben Pfaff 
On Tue, Dec 11, 2018 at 08:19:55PM +, Gregory Smith wrote:
> Hi Ben,
> 
> Thanks for taking a look.
> 
> Ben Pfaff wrote:
> > This code looks pretty optimistic about things that I'm not sure it
> > should depend on.  It uses ALIGNED_CAST in multiple places although I
> > don't see a reason to assume that the data in question is actually
> > aligned.
> 
> Right. I should've spent a little more time thinking this through before
> submitting the revision. I've apparently rotted my brain on x86
> development...
> 
> As for the fix: are the get_unaligned_xxx() functions the right tool to
> use? I looked through pinctrl.c for other examples of unaligned access
> to mimic, and I was suprised not to find any special handling (e.g., the
> extraction of the 4-byte iaid in pinctrl_handle_put_dhcpv6_opts(), or
> the 2-byte query_type in pinctrl_handle_dns_lookup()).
> 
> Out of curiosity, do we have any test coverage for CPU architectures
> that are sensitive to alignment?

We have a few different strategies.  get_unaligned_xxx() is one of them.
Another is get_16aligned_be32() and friends, where we can assume that
data structure are at least aligned on a 16-bit boundary (usually the
case).  Occasionally, we use "packed" annotations to avoid having to do
anything special.  The latter appears to be the case for the dhcpv6 code
you mention.

We have test coverage mostly through Debian builds, which isn't a great
way because Debian lags far behind master, but it's better than nothing.

> > It also appears to read out the DHCP magic cookie before it checks
> > whether the packet is long enough to contain it.
> 
> I think this part was actually correct; the patch advances in_dhcp_ptr
> and does a bounds check before derefercing in_dhcp_cookie.
> 
> >> +const ovs_be32 *in_dhcp_cookie =
> >> +ALIGNED_CAST(const ovs_be32 *, in_dhcp_ptr);
> >> +in_dhcp_ptr += sizeof *in_dhcp_cookie;
> >> +if (in_dhcp_ptr > end || *in_dhcp_cookie != magic_cookie) {

You're right, I misread the code.

Thanks,

Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v5] pinctrl: Check requested IP in DHCPREQUEST messages

2018-12-11 Thread Gregory Smith 
Hi Ben,

Thanks for taking a look.

Ben Pfaff wrote:
> This code looks pretty optimistic about things that I'm not sure it
> should depend on.  It uses ALIGNED_CAST in multiple places although I
> don't see a reason to assume that the data in question is actually
> aligned.

Right. I should've spent a little more time thinking this through before
submitting the revision. I've apparently rotted my brain on x86
development...

As for the fix: are the get_unaligned_xxx() functions the right tool to
use? I looked through pinctrl.c for other examples of unaligned access
to mimic, and I was suprised not to find any special handling (e.g., the
extraction of the 4-byte iaid in pinctrl_handle_put_dhcpv6_opts(), or
the 2-byte query_type in pinctrl_handle_dns_lookup()).

Out of curiosity, do we have any test coverage for CPU architectures
that are sensitive to alignment?

> It also appears to read out the DHCP magic cookie before it checks
> whether the packet is long enough to contain it.

I think this part was actually correct; the patch advances in_dhcp_ptr
and does a bounds check before derefercing in_dhcp_cookie.

>> +const ovs_be32 *in_dhcp_cookie =
>> +ALIGNED_CAST(const ovs_be32 *, in_dhcp_ptr);
>> +in_dhcp_ptr += sizeof *in_dhcp_cookie;
>> +if (in_dhcp_ptr > end || *in_dhcp_cookie != magic_cookie) {

Thanks,
Greg
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v5] pinctrl: Check requested IP in DHCPREQUEST messages

2018-12-10 Thread Ben Pfaff 
On Wed, Dec 05, 2018 at 07:10:13PM +, Gregory Smith wrote:
> See RFC 2131, section 4.3.2. When handling a DHCPREQUEST message, the
> server should validate that the client's requested IP matches the
> offered IP. If not, the server should reply with a DHCPNAK. The client's
> requested IP is either specified as the Requested IP Address (option
> 50), or as the ciaddr, depending on the client's state.
> 
> Signed-off-by: Gregory Smith 
> ---
> 
> v4 -> v5
> 
>  * Fixed clang cast alignment warnings.
> 
> v3 -> v4
> 
>  * Reworked the option-parsing while loop.
> 
> v2 -> v3
> 
>  * Fixed long line.
> 
> v1 -> v2
> 
>  * Refactored DHCP option parsing and packet boundary checks.
> 
>  lib/dhcp.h   |   3 +
>  ovn/controller/pinctrl.c | 129 +---
>  ovn/lib/ovn-l7.h |   9 +++
>  tests/ovn.at | 188 
> +--
>  4 files changed, 264 insertions(+), 65 deletions(-)

Thanks for the patch.

This code looks pretty optimistic about things that I'm not sure it
should depend on.  It uses ALIGNED_CAST in multiple places although I
don't see a reason to assume that the data in question is actually
aligned.  It also appears to read out the DHCP magic cookie before it
checks whether the packet is long enough to contain it.

Please take another look and make sure that this code is validating
everything properly.

Thanks,

Ben.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v5] pinctrl: Check requested IP in DHCPREQUEST messages

2018-12-05 Thread Gregory Smith 
See RFC 2131, section 4.3.2. When handling a DHCPREQUEST message, the
server should validate that the client's requested IP matches the
offered IP. If not, the server should reply with a DHCPNAK. The client's
requested IP is either specified as the Requested IP Address (option
50), or as the ciaddr, depending on the client's state.

Signed-off-by: Gregory Smith 
---

v4 -> v5

 * Fixed clang cast alignment warnings.

v3 -> v4

 * Reworked the option-parsing while loop.

v2 -> v3

 * Fixed long line.

v1 -> v2

 * Refactored DHCP option parsing and packet boundary checks.

 lib/dhcp.h   |   3 +
 ovn/controller/pinctrl.c | 129 +---
 ovn/lib/ovn-l7.h |   9 +++
 tests/ovn.at | 188 +--
 4 files changed, 264 insertions(+), 65 deletions(-)

diff --git a/lib/dhcp.h b/lib/dhcp.h
index 271e0a5..73f593a 100644
--- a/lib/dhcp.h
+++ b/lib/dhcp.h
@@ -54,7 +54,10 @@ BUILD_ASSERT_DECL(DHCP_HEADER_LEN == sizeof(struct 
dhcp_header));
 #define DHCP_MSG_OFFER 2
 #define DHCP_MSG_REQUEST   3
 #define DHCP_MSG_ACK   5
+#define DHCP_MSG_NAK   6
 
+#define DHCP_OPT_PAD   0
+#define DHCP_OPT_REQ_IP50
 #define DHCP_OPT_MSG_TYPE  53
 #define DHCP_OPT_END   255
 
diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
index 56539a8..6b26d82 100644
--- a/ovn/controller/pinctrl.c
+++ b/ovn/controller/pinctrl.c
@@ -608,14 +608,23 @@ pinctrl_handle_put_dhcp_opts(
  *| UDP HEADER  | DHCP HEADER  | 4 Byte DHCP Cookie | DHCP OPTIONS(var 
len)|
  * 
  */
-if (dp_packet_l4_size(pkt_in) < (UDP_HEADER_LEN +
-sizeof (struct dhcp_header) + sizeof(uint32_t) + 3)) {
+
+const char *end = (char *)dp_packet_l4(pkt_in) + dp_packet_l4_size(pkt_in);
+const char *in_dhcp_ptr = dp_packet_get_udp_payload(pkt_in);
+if (!in_dhcp_ptr) {
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
 VLOG_WARN_RL(, "Invalid or incomplete DHCP packet received");
 goto exit;
 }
 
-struct dhcp_header const *in_dhcp_data = dp_packet_get_udp_payload(pkt_in);
+const struct dhcp_header *in_dhcp_data =
+ALIGNED_CAST(const struct dhcp_header *, in_dhcp_ptr);
+in_dhcp_ptr += sizeof *in_dhcp_data;
+if (in_dhcp_ptr > end) {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+VLOG_WARN_RL(, "Invalid or incomplete DHCP packet received");
+goto exit;
+}
 if (in_dhcp_data->op != DHCP_OP_REQUEST) {
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
 VLOG_WARN_RL(, "Invalid opcode in the DHCP packet : %d",
@@ -626,41 +635,101 @@ pinctrl_handle_put_dhcp_opts(
 /* DHCP options follow the DHCP header. The first 4 bytes of the DHCP
  * options is the DHCP magic cookie followed by the actual DHCP options.
  */
-const uint8_t *in_dhcp_opt =
-(const uint8_t *)dp_packet_get_udp_payload(pkt_in) +
-sizeof (struct dhcp_header);
-
 ovs_be32 magic_cookie = htonl(DHCP_MAGIC_COOKIE);
-if (memcmp(in_dhcp_opt, _cookie, sizeof(ovs_be32))) {
+const ovs_be32 *in_dhcp_cookie =
+ALIGNED_CAST(const ovs_be32 *, in_dhcp_ptr);
+in_dhcp_ptr += sizeof *in_dhcp_cookie;
+if (in_dhcp_ptr > end || *in_dhcp_cookie != magic_cookie) {
 static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
 VLOG_WARN_RL(, "DHCP magic cookie not present in the DHCP packet");
 goto exit;
 }
 
-in_dhcp_opt += 4;
+const uint8_t *in_dhcp_msg_type = NULL;
+const ovs_be32 *in_dhcp_request_ip = NULL;
+while (in_dhcp_ptr < end) {
+const struct dhcp_opt_header *in_dhcp_opt =
+(const struct dhcp_opt_header *)in_dhcp_ptr;
+if (in_dhcp_opt->code == DHCP_OPT_END) {
+break;
+}
+if (in_dhcp_opt->code == DHCP_OPT_PAD) {
+in_dhcp_ptr += 1;
+continue;
+}
+
+in_dhcp_ptr += sizeof *in_dhcp_opt + in_dhcp_opt->len;
+if (in_dhcp_ptr > end) {
+break;
+}
+
+switch (in_dhcp_opt->code) {
+case DHCP_OPT_MSG_TYPE:
+if (in_dhcp_opt->len == 1) {
+in_dhcp_msg_type = (const uint8_t *)_dhcp_opt[1];
+}
+break;
+case DHCP_OPT_REQ_IP:
+if (in_dhcp_opt->len == 4) {
+in_dhcp_request_ip =
+ALIGNED_CAST(const ovs_be32 *, _dhcp_opt[1]);
+}
+break;
+default:
+break;
+}
+
+if (in_dhcp_msg_type && in_dhcp_request_ip) {
+break;
+}
+}
+
 /* Check that the DHCP Message Type (opt 53) is present or not with
- * valid values - DHCP_MSG_DISCOVER or DHCP_MSG_REQUEST as the first
- * DHCP option.
+ * valid values - DHCP_MSG_DISCOVER