| Message ID | 20240123141545.2093189-1-dceara@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] pinctrl: dns: Ignore additional additional records. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Hi Dumitru. Two questions: 1) Did you mean to title this as "additional additional records?" Or did you just mean "additional records?" 2) Should this include a test? I'm thinking you could construct a DNS query that includes an EDNS AR and ensure that OVN responds to it. Thanks, Mark Michelson On 1/23/24 09:15, Dumitru Ceara wrote: > EDNS is backwards compatible so it's safe to just ignore additional ARs. > > Reported-at: https://github.com/ovn-org/ovn/issues/228 > Reported-at: https://issues.redhat.com/browse/FDP-222 > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > controller/pinctrl.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index 4992eab089..0be77701ec 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -2885,6 +2885,7 @@ dns_build_ptr_answer( > free(encoded); > } > > +#define DNS_QUERY_TYPE_CLASS_LEN (2 * sizeof(ovs_be16)) > #define DNS_RCODE_SERVER_REFUSE 0x5 > > /* Called with in the pinctrl_handler thread context. */ > @@ -2949,18 +2950,13 @@ pinctrl_handle_dns_lookup( > goto exit; > } > > - /* Check if there is an additional record present, which is unsupported */ > - if (in_dns_header->arcount) { > - VLOG_DBG_RL(&rl, "Received DNS query with additional records, which" > - " is unsupported"); > - goto exit; > - } > - > struct udp_header *in_udp = dp_packet_l4(pkt_in); > size_t udp_len = ntohs(in_udp->udp_len); > size_t l4_len = dp_packet_l4_size(pkt_in); > + uint8_t *l4_start = (uint8_t *) in_udp; > uint8_t *end = (uint8_t *)in_udp + MIN(udp_len, l4_len); > uint8_t *in_dns_data = (uint8_t *)(in_dns_header + 1); > + uint8_t *in_dns_data_start = in_dns_data; > uint8_t *in_queryname = in_dns_data; > uint16_t idx = 0; > struct ds query_name; > @@ -2984,7 +2980,7 @@ pinctrl_handle_dns_lookup( > in_dns_data += idx; > > /* Query should have TYPE and CLASS fields */ > - if (in_dns_data + (2 * sizeof(ovs_be16)) > end) { > + if (in_dns_data + DNS_QUERY_TYPE_CLASS_LEN > end) { > ds_destroy(&query_name); > goto exit; > } > @@ -2998,6 +2994,10 @@ pinctrl_handle_dns_lookup( > goto exit; > } > > + uint8_t *rest = in_dns_data + DNS_QUERY_TYPE_CLASS_LEN; > + uint32_t query_size = rest - in_dns_data_start; > + uint32_t query_l4_size = rest - l4_start; > + > uint64_t dp_key = ntohll(pin->flow_metadata.flow.metadata); > const char *answer_data = NULL; > bool ovn_owned = false; > @@ -3080,7 +3080,7 @@ pinctrl_handle_dns_lookup( > goto exit; > } > > - uint16_t new_l4_size = ntohs(in_udp->udp_len) + dns_answer.size; > + uint16_t new_l4_size = query_l4_size + dns_answer.size; > size_t new_packet_size = pkt_in->l4_ofs + new_l4_size; > struct dp_packet pkt_out; > dp_packet_init(&pkt_out, new_packet_size); > @@ -3117,7 +3117,7 @@ pinctrl_handle_dns_lookup( > out_dns_header->arcount = 0; > > /* Copy the Query section. */ > - dp_packet_put(&pkt_out, dp_packet_data(pkt_in), dp_packet_size(pkt_in)); > + dp_packet_put(&pkt_out, dp_packet_data(pkt_in), query_size); > > /* Copy the answer sections. */ > dp_packet_put(&pkt_out, dns_answer.data, dns_answer.size);
On 1/23/24 22:47, Mark Michelson wrote: > Hi Dumitru. > Hi Mark, > Two questions: > > 1) Did you mean to title this as "additional additional records?" Or did > you just mean "additional records?" > Well, no, I didn't I just expanded AR without thinking. :) > 2) Should this include a test? I'm thinking you could construct a DNS > query that includes an EDNS AR and ensure that OVN responds to it. > I should've added a test, you're right. I'll work on v2, thanks for checking this out! Regards, Dumitru > Thanks, > Mark Michelson > > On 1/23/24 09:15, Dumitru Ceara wrote: >> EDNS is backwards compatible so it's safe to just ignore additional ARs. >> >> Reported-at: https://github.com/ovn-org/ovn/issues/228 >> Reported-at: https://issues.redhat.com/browse/FDP-222 >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> --- >> controller/pinctrl.c | 20 ++++++++++---------- >> 1 file changed, 10 insertions(+), 10 deletions(-) >> >> diff --git a/controller/pinctrl.c b/controller/pinctrl.c >> index 4992eab089..0be77701ec 100644 >> --- a/controller/pinctrl.c >> +++ b/controller/pinctrl.c >> @@ -2885,6 +2885,7 @@ dns_build_ptr_answer( >> free(encoded); >> } >> +#define DNS_QUERY_TYPE_CLASS_LEN (2 * sizeof(ovs_be16)) >> #define DNS_RCODE_SERVER_REFUSE 0x5 >> /* Called with in the pinctrl_handler thread context. */ >> @@ -2949,18 +2950,13 @@ pinctrl_handle_dns_lookup( >> goto exit; >> } >> - /* Check if there is an additional record present, which is >> unsupported */ >> - if (in_dns_header->arcount) { >> - VLOG_DBG_RL(&rl, "Received DNS query with additional records, >> which" >> - " is unsupported"); >> - goto exit; >> - } >> - >> struct udp_header *in_udp = dp_packet_l4(pkt_in); >> size_t udp_len = ntohs(in_udp->udp_len); >> size_t l4_len = dp_packet_l4_size(pkt_in); >> + uint8_t *l4_start = (uint8_t *) in_udp; >> uint8_t *end = (uint8_t *)in_udp + MIN(udp_len, l4_len); >> uint8_t *in_dns_data = (uint8_t *)(in_dns_header + 1); >> + uint8_t *in_dns_data_start = in_dns_data; >> uint8_t *in_queryname = in_dns_data; >> uint16_t idx = 0; >> struct ds query_name; >> @@ -2984,7 +2980,7 @@ pinctrl_handle_dns_lookup( >> in_dns_data += idx; >> /* Query should have TYPE and CLASS fields */ >> - if (in_dns_data + (2 * sizeof(ovs_be16)) > end) { >> + if (in_dns_data + DNS_QUERY_TYPE_CLASS_LEN > end) { >> ds_destroy(&query_name); >> goto exit; >> } >> @@ -2998,6 +2994,10 @@ pinctrl_handle_dns_lookup( >> goto exit; >> } >> + uint8_t *rest = in_dns_data + DNS_QUERY_TYPE_CLASS_LEN; >> + uint32_t query_size = rest - in_dns_data_start; >> + uint32_t query_l4_size = rest - l4_start; >> + >> uint64_t dp_key = ntohll(pin->flow_metadata.flow.metadata); >> const char *answer_data = NULL; >> bool ovn_owned = false; >> @@ -3080,7 +3080,7 @@ pinctrl_handle_dns_lookup( >> goto exit; >> } >> - uint16_t new_l4_size = ntohs(in_udp->udp_len) + dns_answer.size; >> + uint16_t new_l4_size = query_l4_size + dns_answer.size; >> size_t new_packet_size = pkt_in->l4_ofs + new_l4_size; >> struct dp_packet pkt_out; >> dp_packet_init(&pkt_out, new_packet_size); >> @@ -3117,7 +3117,7 @@ pinctrl_handle_dns_lookup( >> out_dns_header->arcount = 0; >> /* Copy the Query section. */ >> - dp_packet_put(&pkt_out, dp_packet_data(pkt_in), >> dp_packet_size(pkt_in)); >> + dp_packet_put(&pkt_out, dp_packet_data(pkt_in), query_size); >> /* Copy the answer sections. */ >> dp_packet_put(&pkt_out, dns_answer.data, dns_answer.size); >
diff --git a/controller/pinctrl.c b/controller/pinctrl.c index 4992eab089..0be77701ec 100644 --- a/controller/pinctrl.c +++ b/controller/pinctrl.c @@ -2885,6 +2885,7 @@ dns_build_ptr_answer( free(encoded); } +#define DNS_QUERY_TYPE_CLASS_LEN (2 * sizeof(ovs_be16)) #define DNS_RCODE_SERVER_REFUSE 0x5 /* Called with in the pinctrl_handler thread context. */ @@ -2949,18 +2950,13 @@ pinctrl_handle_dns_lookup( goto exit; } - /* Check if there is an additional record present, which is unsupported */ - if (in_dns_header->arcount) { - VLOG_DBG_RL(&rl, "Received DNS query with additional records, which" - " is unsupported"); - goto exit; - } - struct udp_header *in_udp = dp_packet_l4(pkt_in); size_t udp_len = ntohs(in_udp->udp_len); size_t l4_len = dp_packet_l4_size(pkt_in); + uint8_t *l4_start = (uint8_t *) in_udp; uint8_t *end = (uint8_t *)in_udp + MIN(udp_len, l4_len); uint8_t *in_dns_data = (uint8_t *)(in_dns_header + 1); + uint8_t *in_dns_data_start = in_dns_data; uint8_t *in_queryname = in_dns_data; uint16_t idx = 0; struct ds query_name; @@ -2984,7 +2980,7 @@ pinctrl_handle_dns_lookup( in_dns_data += idx; /* Query should have TYPE and CLASS fields */ - if (in_dns_data + (2 * sizeof(ovs_be16)) > end) { + if (in_dns_data + DNS_QUERY_TYPE_CLASS_LEN > end) { ds_destroy(&query_name); goto exit; } @@ -2998,6 +2994,10 @@ pinctrl_handle_dns_lookup( goto exit; } + uint8_t *rest = in_dns_data + DNS_QUERY_TYPE_CLASS_LEN; + uint32_t query_size = rest - in_dns_data_start; + uint32_t query_l4_size = rest - l4_start; + uint64_t dp_key = ntohll(pin->flow_metadata.flow.metadata); const char *answer_data = NULL; bool ovn_owned = false; @@ -3080,7 +3080,7 @@ pinctrl_handle_dns_lookup( goto exit; } - uint16_t new_l4_size = ntohs(in_udp->udp_len) + dns_answer.size; + uint16_t new_l4_size = query_l4_size + dns_answer.size; size_t new_packet_size = pkt_in->l4_ofs + new_l4_size; struct dp_packet pkt_out; dp_packet_init(&pkt_out, new_packet_size); @@ -3117,7 +3117,7 @@ pinctrl_handle_dns_lookup( out_dns_header->arcount = 0; /* Copy the Query section. */ - dp_packet_put(&pkt_out, dp_packet_data(pkt_in), dp_packet_size(pkt_in)); + dp_packet_put(&pkt_out, dp_packet_data(pkt_in), query_size); /* Copy the answer sections. */ dp_packet_put(&pkt_out, dns_answer.data, dns_answer.size);
EDNS is backwards compatible so it's safe to just ignore additional ARs. Reported-at: https://github.com/ovn-org/ovn/issues/228 Reported-at: https://issues.redhat.com/browse/FDP-222 Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- controller/pinctrl.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)