diff mbox series

[ovs-dev,v7,1/8] lib: Add non-null assertions to return value of `dp_packet_data`

Message ID 20230415152155.762025-2-jamestiotio@gmail.com
State Changes Requested, archived
Headers show
Series treewide: Fix multiple Coverity defects | expand

Checks

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

Commit Message

James Raphael Tiovalen April 15, 2023, 3:21 p.m. UTC
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(-)

Comments

Simon Horman April 21, 2023, 9:57 a.m. UTC | #1
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...
James Raphael Tiovalen May 1, 2023, 2:12 p.m. UTC | #2
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 mbox series

Patch

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);
 }