On Wed, Oct 29, 2014 at 12:11:18PM -0500, D. Jared Dominguez wrote:
> Can you confirm whether this is still an issue for you? New versions
> of libefivar0 are in testing and unstable. Please test against them.
The segmentation fault still occurs with libefivar0 version 0.15-1 and
efibootmgr version 0.11.0-1.
I had some time for a detailed examination of read_file() this weekend and
it looked worse than I had hoped. The attached patch tries to fix the
following problems:
- It returns 0 on read errors and memory allocation failures, although
the code calling it probably expects a return value of -1 and a failure
reason in errno.
- The type of filesize doesn't match the type of size and bufsize.
- Partial read handling seems to be seriously broken. If I'm not mistaken,
buffer overflows are possible.
- There's no integer overflow check before increasing the buffer size.
The only part of that patch that I could test was the handling of read
errors. With the patch, efibootmgr doesn't crash anymore but reports
efibootmgr: efibootmgr: Input/output error
--
Jan
diff -ru efivar-0.15.orig/src/util.h efivar-0.15/src/util.h
--- efivar-0.15.orig/src/util.h 2014-10-15 15:48:49.000000000 +0200
+++ efivar-0.15/src/util.h 2014-11-03 00:45:18.271719150 +0100
@@ -31,46 +31,58 @@
{
uint8_t *p;
size_t size = 4096;
- int s = 0, filesize = 0;
+ int s = 0;
uint8_t *newbuf;
if (!(newbuf = calloc(size, sizeof (uint8_t))))
return -1;
*buf = newbuf;
+ *bufsize = 0;
do {
- p = *buf + filesize;
- s = read(fd, p, 4096 - s);
+ p = *buf + *bufsize;
+ /* size - *bufsize shouldn't exceed SSIZE_MAX because we're
+ only allocating 4096 bytes at a time. */
+ s = read(fd, p, size - *bufsize);
if (s < 0 && errno == EAGAIN) {
continue;
} else if (s < 0) {
+ int saved_errno = errno;
free(*buf);
*buf = NULL;
*bufsize = 0;
- break;
+ errno = saved_errno;
+ return -1;
}
- filesize += s;
+ *bufsize += s;
/* only exit for empty reads */
if (s == 0) {
break;
- } else if (s == 4096) {
+ }
+ if (*bufsize >= size) {
+ if (size > (size_t)-1 - 4096)
+ {
+ free(*buf);
+ *buf = NULL;
+ *bufsize = 0;
+ errno = ENOMEM;
+ return -1;
+ }
newbuf = realloc(*buf, size + 4096);
if (newbuf == NULL) {
+ int saved_errno = errno;
free(*buf);
*buf = NULL;
*bufsize = 0;
+ errno = saved_errno;
return -1;
}
*buf = newbuf;
memset(*buf + size, '\0', 4096);
- size += s;
- s = 0;
- } else {
- size += s;
+ size += 4096;
}
} while (1);
- *bufsize = filesize;
return 0;
}
