pnor_partition: Handle requests exceeding partition's actual size
Partitions with patch files whose size was less than the partition size
in the ToC could not be completely read by the host. For example when
scanning over the entire PNOR on the host with `cat /dev/mtd0 >
/dev/null` the host would lock up. A trace from mboxd under these
circumstances shows:
[ 1519832857.966501396] Received MBOX command: 4
[ 1519832857.966695620] Host requested flash @ 0x02a44000
[ 1519832857.968642020] Window @ 0x730ce000 for size 0x00024000 maps flash offset 0x02a44000
[ 1519832857.968808728] Writing MBOX response: 1
[ 1519832858.222090630] Received MBOX command: 4
[ 1519832858.222284692] Host requested flash @ 0x02a68000
[ 1519832858.223964544] Window @ 0x73cce000 for size 0x00009000 maps flash offset 0x02a68000
[ 1519832858.224136142] Writing MBOX response: 1
[ 1519832858.435944292] Received MBOX command: 4
[ 1519832858.436138394] Host requested flash @ 0x02a71000
[ 1519832858.437026725] Window @ 0x734ce000 for size 0x00007000 maps flash offset 0x02a71000
[ 1519832858.437195251] Writing MBOX response: 1
[ 1519832858.646768070] Received MBOX command: 4
[ 1519832858.646968637] Host requested flash @ 0x02a78000
[ 1519832858.647567228] Window @ 0x768ce000 for size 0x00001000 maps flash offset 0x02a78000
[ 1519832858.647731755] Writing MBOX response: 1
[ 1519832858.848288015] Received MBOX command: 4
[ 1519832858.848489188] Host requested flash @ 0x02a79000
[ 1519832858.849006404] Window @ 0x758ce000 for size 0x00000000 maps flash offset 0x02a79000
[ 1519832858.849168870] Writing MBOX response: 1
[ 1519832859.048631708] Received MBOX command: 4
[ 1519832859.048827305] Host requested flash @ 0x02a79000
[ 1519832859.049343956] Window @ 0x756ce000 for size 0x00000000 maps flash offset 0x02a79000
[ 1519832859.049503553] Writing MBOX response: 1
[ 1519832859.248950916] Received MBOX command: 4
[ 1519832859.249142069] Host requested flash @ 0x02a79000
[ 1519832859.249649871] Window @ 0x741ce000 for size 0x00000000 maps flash offset 0x02a79000
Of significance are the last three CREATE_READ_WINDOW requests, where
the request succeeds but mboxd reports back a zero-sized window to the
host. The host immediately considers itself done with the window, and
requests a new window offset from the previous by size, which is zero.
Thus it re-requests the same offset, and receives the same zero-sized
window in return.
As a result, firmware gets stuck in an unterminated loop, stealing the
core from Linux, which promptly starts reporting a constant stream of
RCU stall warnings among the rest of the failures. Everyone is
miserable.
The offset in question maps to a partition but not to a valid offset in
the file backing that partition. Resize the backing file to meet the
maximum access address within the limits of the partition size defined
in the ToC. By doing so, we are able to map as much of the partition as
necessary.
However, we're not done. Whilst we no longer crash the host, we still
don't successfully complete the operation the host requested. From
Petitboot:
/ # cat /dev/mtd0 > /dev/null
[ 501.061616288,3] MBOX-FLASH: Bad response code from BMC 2
[ 501.150405995,3] MBOX-FLASH: Error waiting for BMC
cat: read error: Input/output error
/ # echo $?
1
/ #
And the corresponding mboxd trace on the BMC:
[ 1519966031.652036815] Received MBOX command: 4
[ 1519966031.652272613] Host requested flash @ 0x03f1a000
[ 1519966031.652411603] Tried to open read window past flash limit
[ 1519966031.652500088] Couldn't create window mapping for offset 0x03f1a000
[ 1519966031.652607966] Error handling mbox cmd: 4
[ 1519966031.652661421] Writing MBOX response: 2
[ 1519966031.652762229] Error handling MBOX event
The read failure will be fixed in a follow-up patch.
Change-Id: Iffdfb8af6f739df5e6d9c171b584a7244bdb7099
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
diff --git a/mboxd_windows.c b/mboxd_windows.c
index fd679bc..9742180 100644
--- a/mboxd_windows.c
+++ b/mboxd_windows.c
@@ -619,7 +619,12 @@
reset_window(context, cur);
return rc;
}
- /* rc isn't guaranteed to be aligned, so align up */
+ /*
+ * rc isn't guaranteed to be aligned, so align up
+ *
+ * FIXME: This should only be the case for the vpnor ToC now, so handle
+ * it there
+ */
cur->size = align_up(rc, (1ULL << context->block_size_shift));
/* Would like a known value, pick 0xFF to it looks like erased flash */
memset(cur->mem + rc, 0xFF, cur->size - rc);
diff --git a/pnor_partition.cpp b/pnor_partition.cpp
index 390c4d5..84aeb3f 100644
--- a/pnor_partition.cpp
+++ b/pnor_partition.cpp
@@ -10,11 +10,14 @@
#include <phosphor-logging/elog-errors.hpp>
#include <assert.h>
+#include <fcntl.h>
#include <stdint.h>
#include <stdlib.h>
#include <syslog.h>
+#include <sys/types.h>
#include <sys/ioctl.h>
#include <sys/mman.h>
+#include <unistd.h>
#include "common.h"
@@ -86,7 +89,31 @@
return dst;
}
-ssize_t Request::fulfil(void *buf, size_t len, int flags)
+size_t Request::clamp(size_t len)
+{
+ size_t maxAccess = offset + len;
+ size_t partSize = partition.data.size << ctx->block_size_shift;
+ return std::min(maxAccess, partSize) - offset;
+}
+
+void Request::resize(const fs::path &path, size_t len)
+{
+ size_t maxAccess = offset + len;
+ size_t fileSize = fs::file_size(path);
+ if (maxAccess < fileSize)
+ {
+ return;
+ }
+ MSG_DBG("Resizing %s to %zu bytes\n", path.c_str(), maxAccess);
+ int rc = truncate(path.c_str(), maxAccess);
+ if (rc == -1)
+ {
+ MSG_ERR("Failed to resize %s: %d\n", path.c_str(), errno);
+ throw std::system_error(errno, std::system_category());
+ }
+}
+
+size_t Request::fulfil(const fs::path &path, int flags, void *buf, size_t len)
{
if (!(flags == O_RDONLY || flags == O_RDWR))
{
@@ -96,8 +123,6 @@
throw std::invalid_argument(err.str());
}
- fs::path path = getPartitionFilePath(flags);
-
int fd = ::open(path.c_str(), flags);
if (fd == -1)
{
@@ -106,45 +131,29 @@
throw std::system_error(errno, std::system_category());
}
+ size_t fileSize = fs::file_size(path);
int mprot = PROT_READ | ((flags == O_RDWR) ? PROT_WRITE : 0);
- auto map = mmap(NULL, partition.data.actual, mprot, MAP_SHARED, fd, 0);
+ auto map = mmap(NULL, fileSize, mprot, MAP_SHARED, fd, 0);
if (map == MAP_FAILED)
{
close(fd);
- MSG_ERR("Failed to map backing file '%s' for %d bytes: %d\n",
- path.c_str(), partition.data.actual, errno);
+ MSG_ERR("Failed to map backing file '%s' for %zd bytes: %d\n",
+ path.c_str(), fileSize, errno);
throw std::system_error(errno, std::system_category());
}
// copy to the reserved memory area
if (flags == O_RDONLY)
{
- len = std::min(partition.data.actual - offset, len);
- memcpy(buf, (char *)map + offset, len);
+ memset(buf, 0xff, len);
+ memcpy(buf, (char *)map + offset, std::min(len, fileSize));
}
else
{
- // if the asked offset + no of bytes to read is greater
- // then size of the partition file then throw error.
- //
- // FIXME: Don't use .actual, use (.size << ctx->block_size_shift),
- // otherwise we can't grow the size of the data to fill the partition
- if ((base + offset + len) > (base + partition.data.actual))
- {
- munmap(map, partition.data.actual);
- close(fd);
-
- /* FIXME: offset calculation */
- std::stringstream err;
- err << "Request size 0x" << std::hex << len << " from offset 0x"
- << std::hex << offset << " exceeds the partition size 0x"
- << std::hex << partition.data.actual;
- throw OutOfBoundsOffset(err.str());
- }
memcpy((char *)map + offset, buf, len);
set_flash_bytemap(ctx, base + offset, len, FLASH_DIRTY);
}
- munmap(map, partition.data.actual);
+ munmap(map, fileSize);
close(fd);
return len;
diff --git a/pnor_partition.hpp b/pnor_partition.hpp
index 80777ce..eb63a39 100644
--- a/pnor_partition.hpp
+++ b/pnor_partition.hpp
@@ -48,15 +48,44 @@
ssize_t read(void* dst, size_t len)
{
- return fulfil(dst, len, O_RDONLY);
+ len = clamp(len);
+ constexpr auto flags = O_RDONLY;
+ fs::path path = getPartitionFilePath(flags);
+ return fulfil(path, flags, dst, len);
}
ssize_t write(void* dst, size_t len)
{
- return fulfil(dst, len, O_RDWR);
+ if (len != clamp(len))
+ {
+ std::stringstream err;
+ err << "Request size 0x" << std::hex << len << " from offset 0x"
+ << std::hex << offset << " exceeds the partition size 0x"
+ << std::hex << (partition.data.size << ctx->block_size_shift);
+ throw OutOfBoundsOffset(err.str());
+ }
+ constexpr auto flags = O_RDWR;
+ /* Ensure file is at least the size of the maximum access */
+ fs::path path = getPartitionFilePath(flags);
+ resize(path, len);
+ return fulfil(path, flags, dst, len);
}
private:
+ /** @brief Clamp the access length to the maximum supported by the ToC */
+ size_t clamp(size_t len);
+
+ /** @brief Ensure the backing file is sized appropriately for the access
+ *
+ * We need to ensure the file is big enough to satisfy the request so that
+ * mmap() will succeed for the required size.
+ *
+ * @return The valid access length
+ *
+ * Throws: std::system_error
+ */
+ void resize(const std::experimental::filesystem::path& path, size_t len);
+
/** @brief Returns the partition file path associated with the offset.
*
* The search strategy for the partition file depends on the value of the
@@ -97,7 +126,8 @@
* @param[out] dst - The buffer to fill with partition data
* @param[in] len - The length of the destination buffer
*/
- ssize_t fulfil(void* dst, size_t len, int flags);
+ size_t fulfil(const std::experimental::filesystem::path& path, int flags,
+ void* dst, size_t len);
struct mbox_context* ctx;
const pnor_partition& partition;
diff --git a/pnor_partition_table.cpp b/pnor_partition_table.cpp
index 376b191..4ad07bc 100644
--- a/pnor_partition_table.cpp
+++ b/pnor_partition_table.cpp
@@ -154,8 +154,10 @@
for (decltype(numParts) i{}; i < numParts; ++i)
{
const struct pnor_partition& part = table.partitions[i];
+ size_t len = part.data.size;
+
if ((blockOffset >= part.data.base) &&
- (blockOffset < (part.data.base + part.data.size)))
+ (blockOffset < (part.data.base + len)))
{
return part;
}
diff --git a/test/vpnor/Makefile.am.include b/test/vpnor/Makefile.am.include
index 8d0dd65..09a4cc8 100644
--- a/test/vpnor/Makefile.am.include
+++ b/test/vpnor/Makefile.am.include
@@ -274,7 +274,4 @@
%reldir%/create_read_window_partition_invalid \
%reldir%/read_patch \
%reldir%/write_patch_resize
-
-XFAIL_TESTS += %reldir%/read_patch
-XFAIL_TESTS += %reldir%/write_patch_resize
endif