From 1a3e08a7197addb1c07e66c1b1da3286c9bcb140 Mon Sep 17 00:00:00 2001 From: Benoît Ganne Date: Thu, 11 Feb 2021 19:46:43 +0100 Subject: vppinfra: fix memcpy undefined behaviour MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Calling mem{cpy,move} with NULL pointers results in undefined behaviour. This in turns is exploited by GCC. For example, the sequence: memcpy (dst, src, n); if (!src) return; src[0] = 0xcafe; will be optimized as memcpy (dst, src, n); src[0] = 0xcafe; IOW the test for NULL is gone. vec_*() functions sometime call memcpy with NULL pointers and 0 length, triggering this optimization. For example, the sequence: vec_append(v1, v2); len = vec_len(v2); will crash if v2 is NULL, because the test for NULL pointer in vec_len() has been optimized out. This commit fixes occurrences of such undefined behaviour, and also introduces a memcpy wrapper to catch those in debug mode. Type: fix Change-Id: I175e2dd726a883f97cf7de3b15f66d4b237ddefd Signed-off-by: Benoît Ganne --- src/vppinfra/hash.c | 18 ++--- src/vppinfra/memcpy_avx2.h | 2 +- src/vppinfra/memcpy_avx512.h | 2 +- src/vppinfra/memcpy_sse3.h | 2 +- src/vppinfra/string.h | 27 ++++++-- src/vppinfra/vec.h | 152 +++++++++++++++++++++++++------------------ 6 files changed, 123 insertions(+), 80 deletions(-) (limited to 'src') diff --git a/src/vppinfra/hash.c b/src/vppinfra/hash.c index 220c16989de..da37b6e88a7 100644 --- a/src/vppinfra/hash.c +++ b/src/vppinfra/hash.c @@ -548,6 +548,7 @@ lookup (void *v, uword key, enum lookup_opcode op, hash_t *h = hash_header (v); hash_pair_union_t *p = 0; uword found_key = 0; + uword value_bytes; uword i; if (!v) @@ -555,6 +556,7 @@ lookup (void *v, uword key, enum lookup_opcode op, i = key_sum (h, key) & (_vec_len (v) - 1); p = get_pair (v, i); + value_bytes = hash_value_bytes (h); if (hash_is_user (v, i)) { @@ -564,9 +566,8 @@ lookup (void *v, uword key, enum lookup_opcode op, if (op == UNSET) { set_is_user (v, i, 0); - if (old_value) - clib_memcpy_fast (old_value, p->direct.value, - hash_value_bytes (h)); + if (old_value && value_bytes) + clib_memcpy_fast (old_value, p->direct.value, value_bytes); zero_pair (h, &p->direct); } } @@ -598,9 +599,8 @@ lookup (void *v, uword key, enum lookup_opcode op, found_key = p != 0; if (found_key && op == UNSET) { - if (old_value) - clib_memcpy_fast (old_value, &p->direct.value, - hash_value_bytes (h)); + if (old_value && value_bytes) + clib_memcpy_fast (old_value, &p->direct.value, value_bytes); unset_indirect (v, i, &p->direct); @@ -611,12 +611,12 @@ lookup (void *v, uword key, enum lookup_opcode op, } } - if (op == SET && p != 0) + if (op == SET && p != 0 && value_bytes) { /* Save away old value for caller. */ if (old_value && found_key) - clib_memcpy_fast (old_value, &p->direct.value, hash_value_bytes (h)); - clib_memcpy_fast (&p->direct.value, new_value, hash_value_bytes (h)); + clib_memcpy_fast (old_value, &p->direct.value, value_bytes); + clib_memcpy_fast (&p->direct.value, new_value, value_bytes); } if (op == SET) diff --git a/src/vppinfra/memcpy_avx2.h b/src/vppinfra/memcpy_avx2.h index f3f0d3f1943..f7a36f0e5cb 100644 --- a/src/vppinfra/memcpy_avx2.h +++ b/src/vppinfra/memcpy_avx2.h @@ -114,7 +114,7 @@ clib_mov128blocks (u8 * dst, const u8 * src, size_t n) } static inline void * -clib_memcpy_fast (void *dst, const void *src, size_t n) +clib_memcpy_fast_avx2 (void *dst, const void *src, size_t n) { uword dstu = (uword) dst; uword srcu = (uword) src; diff --git a/src/vppinfra/memcpy_avx512.h b/src/vppinfra/memcpy_avx512.h index 1444c271ff1..98dac75beac 100644 --- a/src/vppinfra/memcpy_avx512.h +++ b/src/vppinfra/memcpy_avx512.h @@ -144,7 +144,7 @@ clib_mov512blocks (u8 * dst, const u8 * src, size_t n) } static inline void * -clib_memcpy_fast (void *dst, const void *src, size_t n) +clib_memcpy_fast_avx512 (void *dst, const void *src, size_t n) { uword dstu = (uword) dst; uword srcu = (uword) src; diff --git a/src/vppinfra/memcpy_sse3.h b/src/vppinfra/memcpy_sse3.h index d9e4ac668e9..aea2005d95a 100644 --- a/src/vppinfra/memcpy_sse3.h +++ b/src/vppinfra/memcpy_sse3.h @@ -188,7 +188,7 @@ clib_mov256 (u8 * dst, const u8 * src) }) static inline void * -clib_memcpy_fast (void *dst, const void *src, size_t n) +clib_memcpy_fast_sse3 (void *dst, const void *src, size_t n) { __m128i xmm0, xmm1, xmm2, xmm3, xmm4, xmm5, xmm6, xmm7, xmm8; uword dstu = (uword) dst; diff --git a/src/vppinfra/string.h b/src/vppinfra/string.h index 4f96450ce9e..fb46a0c6a0d 100644 --- a/src/vppinfra/string.h +++ b/src/vppinfra/string.h @@ -46,6 +46,7 @@ #include /* for CLIB_LINUX_KERNEL */ #include +#include #ifdef CLIB_LINUX_KERNEL #include @@ -73,16 +74,30 @@ void clib_memswap (void *_a, void *_b, uword bytes); #ifndef __COVERITY__ #if __AVX512BITALG__ #include +#define clib_memcpy_fast_arch(a, b, c) clib_memcpy_fast_avx512 (a, b, c) #elif __AVX2__ #include +#define clib_memcpy_fast_arch(a, b, c) clib_memcpy_fast_avx2 (a, b, c) #elif __SSSE3__ #include -#else -#define clib_memcpy_fast(a,b,c) memcpy(a,b,c) -#endif -#else /* __COVERITY__ */ -#define clib_memcpy_fast(a,b,c) memcpy(a,b,c) -#endif +#define clib_memcpy_fast_arch(a, b, c) clib_memcpy_fast_sse3 (a, b, c) +#endif /* __AVX512BITALG__ */ +#endif /* __COVERITY__ */ + +#ifndef clib_memcpy_fast_arch +#define clib_memcpy_fast_arch(a, b, c) memcpy (a, b, c) +#endif /* clib_memcpy_fast_arch */ + +static_always_inline void * +clib_memcpy_fast (void *restrict dst, const void *restrict src, size_t n) +{ + ASSERT (dst && src && + "memcpy(src, dst, n) with src == NULL or dst == NULL is undefined " + "behaviour"); + return clib_memcpy_fast_arch (dst, src, n); +} + +#undef clib_memcpy_fast_arch /* c-11 string manipulation variants */ diff --git a/src/vppinfra/vec.h b/src/vppinfra/vec.h index e8146af7098..d19ff998137 100644 --- a/src/vppinfra/vec.h +++ b/src/vppinfra/vec.h @@ -665,13 +665,19 @@ do { \ @param A alignment (may be zero) @return V (value-result macro parameter) */ -#define vec_add_ha(V,E,N,H,A) \ -do { \ - word _v(n) = (N); \ - word _v(l) = vec_len (V); \ - V = _vec_resize ((V), _v(n), (_v(l) + _v(n)) * sizeof ((V)[0]), (H), (A)); \ - clib_memcpy_fast ((V) + _v(l), (E), _v(n) * sizeof ((V)[0])); \ -} while (0) +#define vec_add_ha(V, E, N, H, A) \ + do \ + { \ + word _v (n) = (N); \ + if (PREDICT_TRUE (_v (n) > 0)) \ + { \ + word _v (l) = vec_len (V); \ + V = _vec_resize ((V), _v (n), (_v (l) + _v (n)) * sizeof ((V)[0]), \ + (H), (A)); \ + clib_memcpy_fast ((V) + _v (l), (E), _v (n) * sizeof ((V)[0])); \ + } \ + } \ + while (0) /** \brief Add N elements to end of vector V (no header, unspecified alignment) @@ -819,22 +825,23 @@ do { \ @return V (value-result macro parameter) */ -#define vec_insert_elts_ha(V,E,N,M,H,A) \ -do { \ - word _v(l) = vec_len (V); \ - word _v(n) = (N); \ - word _v(m) = (M); \ - V = _vec_resize ((V), \ - _v(n), \ - (_v(l) + _v(n))*sizeof((V)[0]), \ - (H), (A)); \ - ASSERT (_v(m) <= _v(l)); \ - memmove ((V) + _v(m) + _v(n), \ - (V) + _v(m), \ - (_v(l) - _v(m)) * sizeof ((V)[0])); \ - clib_memcpy_fast ((V) + _v(m), (E), \ - _v(n) * sizeof ((V)[0])); \ -} while (0) +#define vec_insert_elts_ha(V, E, N, M, H, A) \ + do \ + { \ + word _v (n) = (N); \ + if (PREDICT_TRUE (_v (n) > 0)) \ + { \ + word _v (l) = vec_len (V); \ + word _v (m) = (M); \ + V = _vec_resize ((V), _v (n), (_v (l) + _v (n)) * sizeof ((V)[0]), \ + (H), (A)); \ + ASSERT (_v (m) <= _v (l)); \ + memmove ((V) + _v (m) + _v (n), (V) + _v (m), \ + (_v (l) - _v (m)) * sizeof ((V)[0])); \ + clib_memcpy_fast ((V) + _v (m), (E), _v (n) * sizeof ((V)[0])); \ + } \ + } \ + while (0) /** \brief Insert N vector elements starting at element M, insert given elements (no header, unspecified alignment) @@ -902,15 +909,21 @@ do { \ @param V2 vector to append */ -#define vec_append(v1,v2) \ -do { \ - uword _v(l1) = vec_len (v1); \ - uword _v(l2) = vec_len (v2); \ - \ - v1 = _vec_resize ((v1), _v(l2), \ - (_v(l1) + _v(l2)) * sizeof ((v1)[0]), 0, 0); \ - clib_memcpy_fast ((v1) + _v(l1), (v2), _v(l2) * sizeof ((v2)[0])); \ -} while (0) +#define vec_append(v1, v2) \ + do \ + { \ + uword _v (l1) = vec_len (v1); \ + uword _v (l2) = vec_len (v2); \ + \ + if (PREDICT_TRUE (_v (l2) > 0)) \ + { \ + v1 = _vec_resize ((v1), _v (l2), \ + (_v (l1) + _v (l2)) * sizeof ((v1)[0]), 0, 0); \ + clib_memcpy_fast ((v1) + _v (l1), (v2), \ + _v (l2) * sizeof ((v2)[0])); \ + } \ + } \ + while (0) /** \brief Append v2 after v1. Result in v1. Specified alignment. @param V1 target vector @@ -918,31 +931,42 @@ do { \ @param align required alignment */ -#define vec_append_aligned(v1,v2,align) \ -do { \ - uword _v(l1) = vec_len (v1); \ - uword _v(l2) = vec_len (v2); \ - \ - v1 = _vec_resize ((v1), _v(l2), \ - (_v(l1) + _v(l2)) * sizeof ((v1)[0]), 0, align); \ - clib_memcpy_fast ((v1) + _v(l1), (v2), _v(l2) * sizeof ((v2)[0])); \ -} while (0) +#define vec_append_aligned(v1, v2, align) \ + do \ + { \ + uword _v (l1) = vec_len (v1); \ + uword _v (l2) = vec_len (v2); \ + \ + if (PREDICT_TRUE (_v (l2) > 0)) \ + { \ + v1 = _vec_resize ( \ + (v1), _v (l2), (_v (l1) + _v (l2)) * sizeof ((v1)[0]), 0, align); \ + clib_memcpy_fast ((v1) + _v (l1), (v2), \ + _v (l2) * sizeof ((v2)[0])); \ + } \ + } \ + while (0) /** \brief Prepend v2 before v1. Result in v1. @param V1 target vector @param V2 vector to prepend */ -#define vec_prepend(v1,v2) \ -do { \ - uword _v(l1) = vec_len (v1); \ - uword _v(l2) = vec_len (v2); \ - \ - v1 = _vec_resize ((v1), _v(l2), \ - (_v(l1) + _v(l2)) * sizeof ((v1)[0]), 0, 0); \ - memmove ((v1) + _v(l2), (v1), _v(l1) * sizeof ((v1)[0])); \ - clib_memcpy_fast ((v1), (v2), _v(l2) * sizeof ((v2)[0])); \ -} while (0) +#define vec_prepend(v1, v2) \ + do \ + { \ + uword _v (l1) = vec_len (v1); \ + uword _v (l2) = vec_len (v2); \ + \ + if (PREDICT_TRUE (_v (l2) > 0)) \ + { \ + v1 = _vec_resize ((v1), _v (l2), \ + (_v (l1) + _v (l2)) * sizeof ((v1)[0]), 0, 0); \ + memmove ((v1) + _v (l2), (v1), _v (l1) * sizeof ((v1)[0])); \ + clib_memcpy_fast ((v1), (v2), _v (l2) * sizeof ((v2)[0])); \ + } \ + } \ + while (0) /** \brief Prepend v2 before v1. Result in v1. Specified alignment @param V1 target vector @@ -950,17 +974,21 @@ do { \ @param align required alignment */ -#define vec_prepend_aligned(v1,v2,align) \ -do { \ - uword _v(l1) = vec_len (v1); \ - uword _v(l2) = vec_len (v2); \ - \ - v1 = _vec_resize ((v1), _v(l2), \ - (_v(l1) + _v(l2)) * sizeof ((v1)[0]), 0, align); \ - memmove ((v1) + _v(l2), (v1), _v(l1) * sizeof ((v1)[0])); \ - clib_memcpy_fast ((v1), (v2), _v(l2) * sizeof ((v2)[0])); \ -} while (0) - +#define vec_prepend_aligned(v1, v2, align) \ + do \ + { \ + uword _v (l1) = vec_len (v1); \ + uword _v (l2) = vec_len (v2); \ + \ + if (PREDICT_TRUE (_v (l2) > 0)) \ + { \ + v1 = _vec_resize ( \ + (v1), _v (l2), (_v (l1) + _v (l2)) * sizeof ((v1)[0]), 0, align); \ + memmove ((v1) + _v (l2), (v1), _v (l1) * sizeof ((v1)[0])); \ + clib_memcpy_fast ((v1), (v2), _v (l2) * sizeof ((v2)[0])); \ + } \ + } \ + while (0) /** \brief Zero all vector elements. Null-pointer tolerant. @param var Vector to zero -- cgit 1.2.3-korg