Message ID | 20230415152155.762025-2-jamestiotio@gmail.com |
---|---|
State | Changes Requested, archived |
Headers | show |
Series | treewide: Fix multiple Coverity defects | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
ovsrobot/intel-ovs-compilation | success | test: success |
On Sat, Apr 15, 2023 at 11:21:48PM +0800, James Raphael Tiovalen wrote: > This commit adds some `ovs_assert()` checks to the return values of > `dp_packet_data()` to ensure that they are not NULL and to prevent > null-pointer dereferences, which might lead to unwanted crashes. We use > assertions since it should be impossible for `dp_packet_data()` to > return NULL. > > Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com> Hi James, thanks for persisting with this patchset. And thanks for splitting it up. That does make things a lot easier, at least for me. I think that the changes you introduce in this patch are both correct and nice. However, I have two questions: 1. I see that some users of dp_packet_data() are not modified by this patch. I did check all of them, but one example is in lib/bfd.c:bfd_process_packet Is that because they were not flagged by Coverity in your investigation? Or for some other reason (I often miss obvious points). 2. Did you consider moving the ovs_assert() into dp_packet_data(). Perhaps that is more risky because perahps sometimes it can legitimately be NULL. But, OTOH, if it is correct for dp_packet_data() to ever return NULL then it would nicely cover all cases. Unless I am missing something. Which often happens...
Hi Simon, On Fri, Apr 21, 2023 at 5:57 PM Simon Horman <simon.horman@corigine.com> wrote: > > 1. I see that some users of dp_packet_data() are not modified by this patch. > I did check all of them, but one example is in lib/bfd.c:bfd_process_packet > > Is that because they were not flagged by Coverity in your investigation? > Or for some other reason (I often miss obvious points). > To be honest, the reason was that they were not flagged by Coverity when I perform my investigation. If you would like me to add the `ovs_assert()` to the `bfd_process_packet()` function, I can do that since that particular case seems to make sense. I am not sure about other users of `dp_packet_data()` though. > 2. Did you consider moving the ovs_assert() into dp_packet_data(). > Perhaps that is more risky because perahps sometimes it can > legitimately be NULL. > > But, OTOH, if it is correct for dp_packet_data() to ever return NULL > then it would nicely cover all cases. > > Unless I am missing something. Which often happens... Unfortunately, I don't think this can be done as it seems that `dp_packet_data()` can legitimately return NULL in some cases. I tested this by simply adding the `ovs_assert()` into `dp_packet_data()` and 676 tests fail. Best regards, James Raphael Tiovalen
diff --git a/lib/dp-packet.c b/lib/dp-packet.c index ae8ab5800..445cb4761 100644 --- a/lib/dp-packet.c +++ b/lib/dp-packet.c @@ -183,9 +183,12 @@ dp_packet_clone_with_headroom(const struct dp_packet *buffer, size_t headroom) struct dp_packet *new_buffer; uint32_t mark; - new_buffer = dp_packet_clone_data_with_headroom(dp_packet_data(buffer), - dp_packet_size(buffer), - headroom); + const void *data_dp = dp_packet_data(buffer); + ovs_assert(data_dp); + + new_buffer = dp_packet_clone_data_with_headroom(data_dp, + dp_packet_size(buffer), + headroom); /* Copy the following fields into the returned buffer: l2_pad_size, * l2_5_ofs, l3_ofs, l4_ofs, cutlen, packet_type and md. */ memcpy(&new_buffer->l2_pad_size, &buffer->l2_pad_size, @@ -322,8 +325,10 @@ dp_packet_shift(struct dp_packet *b, int delta) : true); if (delta != 0) { - char *dst = (char *) dp_packet_data(b) + delta; - memmove(dst, dp_packet_data(b), dp_packet_size(b)); + const void *data_dp = dp_packet_data(b); + ovs_assert(data_dp); + char *dst = (char *) data_dp + delta; + memmove(dst, data_dp, dp_packet_size(b)); dp_packet_set_data(b, dst); } } diff --git a/lib/netdev-native-tnl.c b/lib/netdev-native-tnl.c index 9abdf5107..c1551aa35 100644 --- a/lib/netdev-native-tnl.c +++ b/lib/netdev-native-tnl.c @@ -43,6 +43,7 @@ #include "seq.h" #include "unaligned.h" #include "unixctl.h" +#include "util.h" #include "openvswitch/vlog.h" VLOG_DEFINE_THIS_MODULE(native_tnl); @@ -221,12 +222,13 @@ netdev_tnl_calc_udp_csum(struct udp_header *udp, struct dp_packet *packet, { uint32_t csum; - if (netdev_tnl_is_header_ipv6(dp_packet_data(packet))) { - csum = packet_csum_pseudoheader6(netdev_tnl_ipv6_hdr( - dp_packet_data(packet))); + void *data_dp = dp_packet_data(packet); + ovs_assert(data_dp); + + if (netdev_tnl_is_header_ipv6(data_dp)) { + csum = packet_csum_pseudoheader6(netdev_tnl_ipv6_hdr(data_dp)); } else { - csum = packet_csum_pseudoheader(netdev_tnl_ip_hdr( - dp_packet_data(packet))); + csum = packet_csum_pseudoheader(netdev_tnl_ip_hdr(data_dp)); } csum = csum_continue(csum, udp, ip_tot_size); @@ -425,7 +427,10 @@ netdev_gre_pop_header(struct dp_packet *packet) struct flow_tnl *tnl = &md->tunnel; int hlen = sizeof(struct eth_header) + 4; - hlen += netdev_tnl_is_header_ipv6(dp_packet_data(packet)) ? + const void *data_dp = dp_packet_data(packet); + ovs_assert(data_dp); + + hlen += netdev_tnl_is_header_ipv6(data_dp) ? IPV6_HEADER_LEN : IP_HEADER_LEN; pkt_metadata_init_tnl(md); diff --git a/lib/pcap-file.c b/lib/pcap-file.c index 3ed7ea488..9f4e2e1e2 100644 --- a/lib/pcap-file.c +++ b/lib/pcap-file.c @@ -284,6 +284,8 @@ ovs_pcap_write(struct pcap_file *p_file, struct dp_packet *buf) struct timeval tv; ovs_assert(dp_packet_is_eth(buf)); + const void *data_dp = dp_packet_data(buf); + ovs_assert(data_dp); xgettimeofday(&tv); prh.ts_sec = tv.tv_sec; @@ -291,7 +293,7 @@ ovs_pcap_write(struct pcap_file *p_file, struct dp_packet *buf) prh.incl_len = dp_packet_size(buf); prh.orig_len = dp_packet_size(buf); ignore(fwrite(&prh, sizeof prh, 1, p_file->file)); - ignore(fwrite(dp_packet_data(buf), dp_packet_size(buf), 1, p_file->file)); + ignore(fwrite(data_dp, dp_packet_size(buf), 1, p_file->file)); fflush(p_file->file); }
This commit adds some `ovs_assert()` checks to the return values of `dp_packet_data()` to ensure that they are not NULL and to prevent null-pointer dereferences, which might lead to unwanted crashes. We use assertions since it should be impossible for `dp_packet_data()` to return NULL. Signed-off-by: James Raphael Tiovalen <jamestiotio@gmail.com> --- lib/dp-packet.c | 15 ++++++++++----- lib/netdev-native-tnl.c | 17 +++++++++++------ lib/pcap-file.c | 4 +++- 3 files changed, 24 insertions(+), 12 deletions(-)