On 3 Aug 2023, at 18:19, 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.
Thanks for following up on this! I have some style comments below.
Cheers,
Eelco
> Signed-off-by: James Raphael Tiovalen <[email protected]>
> Reviewed-by: Simon Horman <[email protected]>
> ---
> lib/dp-packet.c | 15 ++++++++++-----
> lib/netdev-native-tnl.c | 6 +++++-
> lib/pcap-file.c | 4 +++-
> 3 files changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> index 27114a9a9..6e3a8297a 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -187,9 +187,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);
How about the following style change?
@@ -184,10 +184,10 @@ dp_packet_clone(const struct dp_packet *buffer)
struct dp_packet *
dp_packet_clone_with_headroom(const struct dp_packet *buffer, size_t headroom)
{
+ const void *data_dp = dp_packet_data(buffer);
struct dp_packet *new_buffer;
uint32_t mark;
- const void *data_dp = dp_packet_data(buffer);
ovs_assert(data_dp);
new_buffer = dp_packet_clone_data_with_headroom(data_dp,
> /* 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,
> @@ -326,8 +329,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);
How about the following style change?
@@ -330,8 +330,10 @@ dp_packet_shift(struct dp_packet *b, int delta)
if (delta != 0) {
const void *data_dp = dp_packet_data(b);
- ovs_assert(data_dp);
char *dst = (char *) data_dp + delta;
+
+ ovs_assert(data_dp);
+
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 715bbab2b..311ba6895 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);
> @@ -419,7 +420,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) ?
How about the following style change?
@@ -416,11 +416,11 @@ parse_gre_header(struct dp_packet *packet,
struct dp_packet *
netdev_gre_pop_header(struct dp_packet *packet)
{
+ const void *data_dp = dp_packet_data(packet);
struct pkt_metadata *md = &packet->md;
struct flow_tnl *tnl = &md->tunnel;
int hlen = sizeof(struct eth_header) + 4;
- 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);
>
How about the following style change?
@@ -280,11 +280,11 @@ ovs_pcap_read(struct pcap_file *p_file, struct dp_packet
**bufp,
void
ovs_pcap_write(struct pcap_file *p_file, struct dp_packet *buf)
{
+ const void *data_dp = dp_packet_data(buf);
struct pcaprec_hdr prh;
struct timeval tv;
ovs_assert(dp_packet_is_eth(buf));
- const void *data_dp = dp_packet_data(buf);
ovs_assert(data_dp);
xgettimeofday(&tv);
> 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.41.0
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
