[ovs-dev] [PATCH v12 1/8] lib: Add non-null assertions to some return values of `dp_packet_data`
This commit adds some `ovs_assert()` checks to some 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 these calls to
`dp_packet_data()` to return NULL.
Signed-off-by: James Raphael Tiovalen
Reviewed-by: Simon Horman
---
lib/dp-packet.c | 15 ++-
lib/netdev-native-tnl.c | 17 +++--
lib/pcap-file.c | 4 +++-
3 files changed, 24 insertions(+), 12 deletions(-)
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 c2c6ca559..7781b162d 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);
@@ -222,12 +223,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);
@@ -428,7 +430,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);
}
--
2.40.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v12 1/8] lib: Add non-null assertions to some return values of `dp_packet_data`
On 13 Jun 2023, at 20:34, James Raphael Tiovalen wrote:
> This commit adds some `ovs_assert()` checks to some 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 these calls to
> `dp_packet_data()` to return NULL.
>
> Signed-off-by: James Raphael Tiovalen
> Reviewed-by: Simon Horman
This patch looks good, however, it needs a re-work due to changes to tunnel
checksum handling.
//Eelco
> ---
> lib/dp-packet.c | 15 ++-
> lib/netdev-native-tnl.c | 17 +++--
> lib/pcap-file.c | 4 +++-
> 3 files changed, 24 insertions(+), 12 deletions(-)
>
> 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 c2c6ca559..7781b162d 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);
> @@ -222,12 +223,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);
> @@ -428,7 +430,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);
> }
>
> --
> 2.40.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswit
Re: [ovs-dev] [PATCH v12 1/8] lib: Add non-null assertions to some return values of `dp_packet_data`
On Tue, Jul 11, 2023 at 5:50 PM Eelco Chaudron wrote: > > This patch looks good, however, it needs a re-work due to changes to tunnel > checksum handling. > > //Eelco > Apologies for the late reply. I have been busy with other work. I will fix the merge conflict in the next patch version. Best regards, James Raphael Tiovalen ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
