[Toybox] [PATCH 2/2] Detect all errors with xreadall/xwrite.

[Samanta Navarro]

If xreadall is called with SIZE_MAX as length, then no error is
detected.

The problem is that readall return value is not explicitly tested for
-1. If len is SIZE_MAX and readall returns -1, then the return value is
promoted to size_t and both values are considered to be equal.

It is unsafe to call xreadall with SIZE_MAX because it ultimately means
that no bound checking for buf is in place. 64 bit systems return EFAULT
when SIZE_MAX reads are performed but 32 bit systems actually start
reading!

I have added an if statement to protect 32 bit systems from such a read
by following style of 64 bit error handling.

The same logic applies to xwrite as well.
---
Proof of Concept (compile Toybox with ASAN=1):

cat > poc.tar.xz.b64 << EOF
/Td6WFoAAATm1rRGAgAhARwAAAAQz1jM4Cf/AGRdADedyhr9nOMBzEAy3Ecv/5ryFigBxe/ACXcH
zFn6/mBaXBzyT3AoV2CbUJCO0Th3OivX+tc868G+6vrnM9H+RXoklVlEq3qM8k/nFHmeMuvp4jlP
9rIYPbgGUyM+SMXXI1gbhQAAihVHNwEYT0EAAYABgFAAAD5xYGGxxGf7AgAAAAAEWVo=
EOF
base64 -d poc.tar.xz.b64 | xz -d > poc.tar

tar -tf poc.tar
---
 lib/xwrap.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/lib/xwrap.c b/lib/xwrap.c
index a58707cc..9d89efeb 100644
--- a/lib/xwrap.c
+++ b/lib/xwrap.c
@@ -512,6 +512,11 @@ size_t xread(int fd, void *buf, size_t len)
 
 void xreadall(int fd, void *buf, size_t len)
 {
+  // Avoid readall with invalid len on all architectures.
+  if (len == SIZE_MAX) {
+    errno = EFAULT;
+    perror_exit("xreadall");
+  }
   if (len != readall(fd, buf, len)) perror_exit("xreadall");
 }
 
@@ -521,6 +526,11 @@ void xreadall(int fd, void *buf, size_t len)
 
 void xwrite(int fd, void *buf, size_t len)
 {
+  // Avoid writeall with invalid len on all architectures.
+  if (len == SIZE_MAX) {
+    errno = EFAULT;
+    perror_exit("xwriteall");
+  }
   if (len != writeall(fd, buf, len)) perror_exit("xwrite");
 }
 
-- 
2.32.0