librbd: readv/writev fix iovecs length computation overflow

iovec have unsigned length (size_t) and before this patch the
total length was computed by adding iovec's length to a signed
length variable (ssize_t). While the code checked if the resulting
length was negative on overflow, the case where length is positive
after overflow was not checked. This patch fixes the overflow check
by changing length to unsigned size_t.

Additionally, this patch fixes the case where some iovecs have been
added to the bufferlist and the aio completion has been blocked, but
adding an additional iovec fails because of overflow. This leads to
the UserBufferDeleter trying to unblock the completion on destruction
of the bufferlist but asserting because the completion was never
armed. We avoid this by first computing the total length and checking
for overflows and iovcnt before adding them to the bufferlist.

Signed-off-by: Jonas Pfefferle <pepperjo@japf.ch>
(cherry picked from commit e50405ef857f487bc1c104bbf3e8859ea099a0c4)

Conflicts:
	src/librbd/librbd.cc [ commit b12b8c447a41 ("librbd: switch
	  to new api::Io dispatch helper methods") not in octopus ]

diff --git a/src/librbd/librbd.cc b/src/librbd/librbd.cc
index 438b58716837502fe4a66e1800b9d912ab24651c..7af9cfeace895f73563e03bf2f31a020c201f82a 100644 (file)
--- a/src/librbd/librbd.cc
+++ b/src/librbd/librbd.cc
@@ -102,6 +102,40 @@ static auto create_write_raw(librbd::ImageCtx *ictx, const char *buf,
       deleter(new UserBufferDeleter(ictx->cct, aio_completion))));
 }
 
+static int get_iovec_length(const struct iovec *iov, int iovcnt, size_t &len)
+{
+  len = 0;
+
+  if (iovcnt <= 0) {
+    return -EINVAL;
+  }
+
+  for (int i = 0; i < iovcnt; ++i) {
+    const struct iovec &io = iov[i];
+    // check for overflow
+    if (len + io.iov_len < len) {
+      return -EINVAL;
+    }
+    len += io.iov_len;
+  }
+
+  return 0;
+}
+
+static bufferlist iovec_to_bufferlist(librbd::ImageCtx *ictx,
+                                      const struct iovec *iov,
+                                      int iovcnt,
+                                      librbd::io::AioCompletion* aio_completion)
+{
+  bufferlist bl;
+  for (int i = 0; i < iovcnt; ++i) {
+    const struct iovec &io = iov[i];
+    bl.push_back(create_write_raw(ictx, static_cast<char*>(io.iov_base),
+                                  io.iov_len, aio_completion));
+  }
+  return bl;
+}
+
 CephContext* get_cct(IoCtx &io_ctx) {
   return reinterpret_cast<CephContext*>(io_ctx.cct());
 }
@@ -5980,30 +6014,16 @@ extern "C" int rbd_aio_writev(rbd_image_t image, const struct iovec *iov,
   librbd::ImageCtx *ictx = (librbd::ImageCtx *)image;
   librbd::RBD::AioCompletion *comp = (librbd::RBD::AioCompletion *)c;
 
-  // convert the scatter list into a bufferlist
-  auto aio_completion = get_aio_completion(comp);
-  ssize_t len = 0;
-  bufferlist bl;
-  for (int i = 0; i < iovcnt; ++i) {
-    const struct iovec &io = iov[i];
-    len += io.iov_len;
-    if (len < 0) {
-      break;
-    }
-
-    bl.push_back(create_write_raw(ictx, static_cast<char*>(io.iov_base),
-                                  io.iov_len, aio_completion));
-  }
-
-  int r = 0;
-  if (iovcnt <= 0 || len < 0) {
-    r = -EINVAL;
-  }
+  size_t len;
+  int r = get_iovec_length(iov, iovcnt, len);
 
   tracepoint(librbd, aio_write_enter, ictx, ictx->name.c_str(),
              ictx->snap_name.c_str(), ictx->read_only, off, len, NULL,
              comp->pc);
+
   if (r == 0) {
+    auto aio_completion = get_aio_completion(comp);
+    auto bl = iovec_to_bufferlist(ictx, iov, iovcnt, aio_completion);
     ictx->io_work_queue->aio_write(aio_completion, off, len, std::move(bl), 0);
   }
   tracepoint(librbd, aio_write_exit, r);
@@ -6053,18 +6073,8 @@ extern "C" int rbd_aio_readv(rbd_image_t image, const struct iovec *iov,
   librbd::ImageCtx *ictx = (librbd::ImageCtx *)image;
   librbd::RBD::AioCompletion *comp = (librbd::RBD::AioCompletion *)c;
 
-  ssize_t len = 0;
-  for (int i = 0; i < iovcnt; ++i) {
-    len += iov[i].iov_len;
-    if (len < 0) {
-      break;
-    }
-  }
-
-  int r = 0;
-  if (iovcnt == 0 || len < 0) {
-    r = -EINVAL;
-  }
+  size_t len;
+  int r = get_iovec_length(iov, iovcnt, len);
 
   tracepoint(librbd, aio_read_enter, ictx, ictx->name.c_str(),
              ictx->snap_name.c_str(), ictx->read_only, off, len, NULL,

diff --git a/src/test/librbd/test_librbd.cc b/src/test/librbd/test_librbd.cc
index dffca086d3ddd8cadfb0f9b19e53963896dafadf..254a27472317fddc75cdf42481bdc61bee6358df 100644 (file)
--- a/src/test/librbd/test_librbd.cc
+++ b/src/test/librbd/test_librbd.cc
@@ -39,6 +39,7 @@
 #include <set>
 #include <thread>
 #include <vector>
+#include <limits>
 
 #include "test/librados/test.h"
 #include "test/librados/test_cxx.h"
@@ -2313,8 +2314,10 @@ TEST_F(TestLibRBD, TestScatterGatherIO)
   ASSERT_EQ(0, rbd_open(ioctx, name.c_str(), &image, NULL));
 
   std::string write_buffer("This is a test");
+  // These iovecs should produce a length overflow
   struct iovec bad_iovs[] = {
-    {.iov_base = NULL, .iov_len = static_cast<size_t>(-1)}
+    {.iov_base = &write_buffer[0], .iov_len = 5},
+    {.iov_base = NULL, .iov_len = std::numeric_limits<size_t>::max()}
   };
   struct iovec write_iovs[] = {
     {.iov_base = &write_buffer[0],  .iov_len = 5},
@@ -2326,7 +2329,7 @@ TEST_F(TestLibRBD, TestScatterGatherIO)
   rbd_completion_t comp;
   rbd_aio_create_completion(NULL, NULL, &comp);
   ASSERT_EQ(-EINVAL, rbd_aio_writev(image, write_iovs, 0, 0, comp));
-  ASSERT_EQ(-EINVAL, rbd_aio_writev(image, bad_iovs, 1, 0, comp));
+  ASSERT_EQ(-EINVAL, rbd_aio_writev(image, bad_iovs, 2, 0, comp));
   ASSERT_EQ(0, rbd_aio_writev(image, write_iovs,
                               sizeof(write_iovs) / sizeof(struct iovec),
                               1<<order, comp));
@@ -2343,7 +2346,7 @@ TEST_F(TestLibRBD, TestScatterGatherIO)
 
   rbd_aio_create_completion(NULL, NULL, &comp);
   ASSERT_EQ(-EINVAL, rbd_aio_readv(image, read_iovs, 0, 0, comp));
-  ASSERT_EQ(-EINVAL, rbd_aio_readv(image, bad_iovs, 1, 0, comp));
+  ASSERT_EQ(-EINVAL, rbd_aio_readv(image, bad_iovs, 2, 0, comp));
   ASSERT_EQ(0, rbd_aio_readv(image, read_iovs,
                              sizeof(read_iovs) / sizeof(struct iovec),
                              1<<order, comp));