[iobuf] Improve robustness of I/O buffer allocation

Guard against various corner cases (such as zero-length buffers, zero
alignments, and integer overflow when rounding up allocation lengths
and alignments) and ensure that the struct io_buffer is correctly
aligned even when the caller requests a non-zero alignment for the I/O
buffer itself.

Add self-tests to verify that the resulting alignments and lengths are
correct for a range of allocations.

Signed-off-by: Michael Brown <mcb30@ipxe.org>

diff --git a/src/core/iobuf.c b/src/core/iobuf.c
index 3e52ada4ff4b291db8668a1376cae428dadb8720..0ee53e0388ad5b42117cc287c7cb6a1ab64744b0 100644 (file)
--- a/src/core/iobuf.c
+++ b/src/core/iobuf.c
@@ -47,20 +47,45 @@ FILE_LICENCE ( GPL2_OR_LATER_OR_UBDL );
  */
 struct io_buffer * alloc_iob_raw ( size_t len, size_t align, size_t offset ) {
        struct io_buffer *iobuf;
+       size_t padding;
+       size_t threshold;
+       unsigned int align_log2;
        void *data;
 
-       /* Align buffer length to ensure that struct io_buffer is aligned */
-       len = ( len + __alignof__ ( *iobuf ) - 1 ) &
-               ~( __alignof__ ( *iobuf ) - 1 );
+       /* Calculate padding required below alignment boundary to
+        * ensure that a correctly aligned inline struct io_buffer
+        * could fit (regardless of the requested offset).
+        */
+       padding = ( sizeof ( *iobuf ) + __alignof__ ( *iobuf ) - 1 );
 
-       /* Round up alignment to the nearest power of two */
-       align = ( 1 << fls ( align - 1 ) );
+       /* Round up requested alignment to at least the size of the
+        * padding, to simplify subsequent calculations.
+        */
+       if ( align < padding )
+               align = padding;
 
-       /* Allocate buffer plus descriptor as a single unit, unless
-        * doing so will push the total size over the alignment
-        * boundary.
+       /* Round up alignment to the nearest power of two, avoiding
+        * a potentially undefined shift operation.
         */
-       if ( ( len + sizeof ( *iobuf ) ) <= align ) {
+       align_log2 = fls ( align - 1 );
+       if ( align_log2 >= ( 8 * sizeof ( align ) ) )
+               return NULL;
+       align = ( 1UL << align_log2 );
+
+       /* Calculate length threshold */
+       assert ( align >= padding );
+       threshold = ( align - padding );
+
+       /* Allocate buffer plus an inline descriptor as a single unit,
+        * unless doing so would push the total size over the
+        * alignment boundary.
+        */
+       if ( len <= threshold ) {
+
+               /* Round up buffer length to ensure that struct
+                * io_buffer is aligned.
+                */
+               len += ( ( - len - offset ) & ( __alignof__ ( *iobuf ) - 1 ) );
 
                /* Allocate memory for buffer plus descriptor */
                data = malloc_dma_offset ( len + sizeof ( *iobuf ), align,

diff --git a/src/tests/iobuf_test.c b/src/tests/iobuf_test.c
new file mode 100644 (file)
index 0000000..a417c2e
--- /dev/null
+++ b/src/tests/iobuf_test.c
@@ -0,0 +1,136 @@
+/*
+ * Copyright (C) 2016 Michael Brown <mbrown@fensystems.co.uk>.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA
+ * 02110-1301, USA.
+ *
+ * You can also choose to distribute this program under the terms of
+ * the Unmodified Binary Distribution Licence (as given in the file
+ * COPYING.UBDL), provided that you have satisfied its requirements.
+ */
+
+FILE_LICENCE ( GPL2_OR_LATER_OR_UBDL );
+
+/** @file
+ *
+ * I/O buffer tests
+ *
+ */
+
+/* Forcibly enable assertions */
+#undef NDEBUG
+
+#include <stdint.h>
+#include <string.h>
+#include <assert.h>
+#include <ipxe/iobuf.h>
+#include <ipxe/io.h>
+#include <ipxe/test.h>
+
+/* Forward declaration */
+struct self_test iobuf_test __self_test;
+
+/**
+ * Report I/O buffer allocation test result
+ *
+ * @v len              Required length of buffer
+ * @v align            Physical alignment
+ * @v offset           Offset from physical alignment
+ * @v file             Test code file
+ * @v line             Test code line
+ */
+static inline void alloc_iob_okx ( size_t len, size_t align, size_t offset,
+                                  const char *file, unsigned int line ) {
+       struct io_buffer *iobuf;
+
+       /* Allocate I/O buffer */
+       iobuf = alloc_iob_raw ( len, align, offset );
+       okx ( iobuf != NULL, file, line );
+       DBGC ( &iobuf_test, "IOBUF %p (%#08lx+%#zx) for %#zx align %#zx "
+              "offset %#zx\n", iobuf, virt_to_phys ( iobuf->data ),
+              iob_tailroom ( iobuf ), len, align, offset );
+
+       /* Validate requested length and alignment */
+       okx ( ( ( ( intptr_t ) iobuf ) & ( __alignof__ ( *iobuf ) - 1 ) ) == 0,
+               file, line );
+       okx ( iob_tailroom ( iobuf ) >= len, file, line );
+       okx ( ( ( align == 0 ) ||
+               ( ( virt_to_phys ( iobuf->data ) & ( align - 1 ) ) ==
+                 ( offset & ( align - 1 ) ) ) ), file, line );
+
+       /* Overwrite entire content of I/O buffer (for Valgrind) */
+       memset ( iob_put ( iobuf, len ), 0x55, len );
+
+       /* Free I/O buffer */
+       free_iob ( iobuf );
+}
+#define alloc_iob_ok( len, align, offset ) \
+       alloc_iob_okx ( len, align, offset, __FILE__, __LINE__ )
+
+/**
+ * Report I/O buffer allocation failure test result
+ *
+ * @v len              Required length of buffer
+ * @v align            Physical alignment
+ * @v offset           Offset from physical alignment
+ * @v file             Test code file
+ * @v line             Test code line
+ */
+static inline void alloc_iob_fail_okx ( size_t len, size_t align, size_t offset,
+                                       const char *file, unsigned int line ) {
+       struct io_buffer *iobuf;
+
+       /* Allocate I/O buffer */
+       iobuf = alloc_iob_raw ( len, align, offset );
+       okx ( iobuf == NULL, file, line );
+}
+#define alloc_iob_fail_ok( len, align, offset ) \
+       alloc_iob_fail_okx ( len, align, offset, __FILE__, __LINE__ )
+
+/**
+ * Perform I/O buffer self-tests
+ *
+ */
+static void iobuf_test_exec ( void ) {
+
+       /* Check zero-length allocations */
+       alloc_iob_ok ( 0, 0, 0 );
+       alloc_iob_ok ( 0, 0, 1 );
+       alloc_iob_ok ( 0, 1, 0 );
+       alloc_iob_ok ( 0, 1024, 0 );
+       alloc_iob_ok ( 0, 139, -17 );
+
+       /* Check various sensible allocations */
+       alloc_iob_ok ( 1, 0, 0 );
+       alloc_iob_ok ( 16, 16, 0 );
+       alloc_iob_ok ( 64, 0, 0 );
+       alloc_iob_ok ( 65, 0, 0 );
+       alloc_iob_ok ( 65, 1024, 19 );
+       alloc_iob_ok ( 1536, 1536, 0 );
+       alloc_iob_ok ( 2048, 2048, 0 );
+       alloc_iob_ok ( 2048, 2048, -10 );
+
+       /* Excessively large or excessively aligned allocations should fail */
+       alloc_iob_fail_ok ( -1UL, 0, 0 );
+       alloc_iob_fail_ok ( -1UL, 1024, 0 );
+       alloc_iob_fail_ok ( 0, -1UL, 0 );
+       alloc_iob_fail_ok ( 1024, -1UL, 0 );
+}
+
+/** I/O buffer self-test */
+struct self_test iobuf_test __self_test = {
+       .name = "iobuf",
+       .exec = iobuf_test_exec,
+};

diff --git a/src/tests/tests.c b/src/tests/tests.c
index 54ce8667791882cdc8151efa01c040f01866ef9b..2e67043e642ac8a1ae142f1161915e97c29d352a 100644 (file)
--- a/src/tests/tests.c
+++ b/src/tests/tests.c
@@ -67,3 +67,4 @@ REQUIRE_OBJECT ( profile_test );
 REQUIRE_OBJECT ( setjmp_test );
 REQUIRE_OBJECT ( pccrc_test );
 REQUIRE_OBJECT ( linebuf_test );
+REQUIRE_OBJECT ( iobuf_test );