From: Peter Krempa <pkrempa@redhat.com> The returned class string from the kernel isn't guaranteed to be always 9 bytes, thus the write to the buffer could happen beyond the guaranteed length. Since 'virFileReadAll' already NUL-terminates the buffer just delete the redundant overwrite. This fixes an invalid write beyond the end of the buffer happening since 458c6a281001d51fd9796 where the returned buffer is shortened. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virpci.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/util/virpci.c b/src/util/virpci.c index ca6f2e8210..2e32ed17ff 100644 --- a/src/util/virpci.c +++ b/src/util/virpci.c @@ -487,7 +487,6 @@ virPCIDeviceReadClass(virPCIDevice *dev, uint16_t *device_class) if (virFileReadAll(path, 9, &id_str) < 0) return -1; - id_str[8] = '\0'; if (virStrToLong_ui(id_str, NULL, 16, &value) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unusual value in %1$s/devices/%2$s/class: %3$s"), -- 2.53.0

From: Peter Krempa <pkrempa@redhat.com> The buffer is already terminated. Luckily the last character in the buffer was a newline so no information was mangled. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virnetdevip.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/util/virnetdevip.c b/src/util/virnetdevip.c index 8786bb236e..c82125b706 100644 --- a/src/util/virnetdevip.c +++ b/src/util/virnetdevip.c @@ -547,10 +547,6 @@ virNetDevIPCheckIPv6Forwarding(void) return false; } - /* Dropping the last character to stop the loop */ - if (len > 0) - buf[len-1] = '\0'; - cur = buf; while (cur) { char route[33], flags[9], iface[9]; -- 2.53.0

From: Peter Krempa <pkrempa@redhat.com> Document both the behaviour if requested length isn't enough to read the file as well as the semantics of NUL-termination of the buffer. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfile.c | 140 ++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 126 insertions(+), 14 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index fbcaf15429..bc3faedd4e 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1534,10 +1534,27 @@ saferead_lim(int fd, size_t max_len, size_t *length) } -/* A wrapper around saferead_lim that merely stops reading at the - * specified maximum size. */ +/** + * virFileReadHeaderQuiet: + * @fd: file descriptor to read + * @maxlen: maximum amount of bytes to read + * @buf: filled with allocated buffer containing read data + * + * Reads up to @maxlen bytes from @fd and fills @buf with pointer to the + * read contents. + * + * The buffer @buf is guaranteed to be terminated with a NUL ('\0') byte one + * byte beyond the read contents of the file. The NUL byte is not included in + * the returned amount of bytes read. Caller is responsible for freeing the + * buffer. + * + * Returns number of bytes actually read from the fd on success, -1 on error + * and doesn't raise a libvirt error. + */ int -virFileReadHeaderFD(int fd, int maxlen, char **buf) +virFileReadHeaderFD(int fd, + int maxlen, + char **buf) { size_t len; char *s; @@ -1554,6 +1571,26 @@ virFileReadHeaderFD(int fd, int maxlen, char **buf) } +/** + * virFileReadHeaderQuiet: + * @path: file to read + * @maxlen: maximum length of file to read + * @buf: filled with allocated buffer containing read data + * + * Reads up to @maxlen bytes from file @path and fills @buf with pointer to the + * read contents. + * + * If file @path is longer than @maxlen the buffer contains only first @maxlen + * bytes read from the file. + * + * The buffer @buf is guaranteed to be terminated with a NUL ('\0') byte one + * byte beyond the read contents of the file. The NUL byte is not included in + * the returned amount of bytes read. Caller is responsible for freeing the + * buffer. + * + * Returns number of bytes actually read from file on success, -1 on error + * and doesn't raise a libvirt error. + */ int virFileReadHeaderQuiet(const char *path, int maxlen, @@ -1573,10 +1610,29 @@ virFileReadHeaderQuiet(const char *path, } -/* A wrapper around saferead_lim that maps a failure due to - exceeding the maximum size limitation to EOVERFLOW. */ +/** + * virFileReadLimFD: + * @fd: file descriptor to read + * @maxlen: maximum amount of bytes to read + * @buf: filled with allocated buffer containing read data + * + * Reads the whole contents from @fd and fills @buf with pointer to the read + * contents. + * + * If @fd allowed reading more than @maxlen bytes an error is returned. + * + * The buffer @buf is guaranteed to be terminated with a NUL ('\0') byte one + * byte beyond the read contents of the file. The NUL byte is not included in + * the returned amount of bytes read. Caller is responsible for freeing the + * buffer. + * + * Returns number of bytes actually read from @fd on success, -1 on error and + * doesn't raise a libvirt error. + */ int -virFileReadLimFD(int fd, int maxlen, char **buf) +virFileReadLimFD(int fd, + int maxlen, + char **buf) { size_t len; char *s; @@ -1599,8 +1655,29 @@ virFileReadLimFD(int fd, int maxlen, char **buf) return len; } + +/** + * virFileReadAll: + * @path: file to read + * @maxlen: maximum length of file to read + * @buf: filled with allocated buffer containing read data + * + * Reads the whole file @path and fills @buf with pointer to the read contents. + * + * If file @path is longer than @maxlen error is returned. + * + * The buffer @buf is guaranteed to be terminated with a NUL ('\0') byte one + * byte beyond the read contents of the file. The NUL byte is not included in + * the returned amount of bytes read. Caller is responsible for freeing the + * buffer. + * + * Returns number of bytes actually read from file on success, -1 on error and + * raises a libvirt error. + */ int -virFileReadAll(const char *path, int maxlen, char **buf) +virFileReadAll(const char *path, + int maxlen, + char **buf) { int fd; int len; @@ -1621,8 +1698,29 @@ virFileReadAll(const char *path, int maxlen, char **buf) return len; } + +/** + * virFileReadAllQuiet: + * @path: file to read + * @maxlen: maximum length of file to read + * @buf: filled with allocated buffer containing read data + * + * Reads the whole file @path and fills @buf with pointer to the read contents. + * + * If file @path is longer than @maxlen error is returned. + * + * The buffer @buf is guaranteed to be terminated with a NUL ('\0') byte one + * byte beyond the read contents of the file. The NUL byte is not included in + * the returned amount of bytes read. Caller is responsible for freeing the + * buffer. + * + * Returns number of bytes actually read from file on success, -errno on error + * and doesn't raise a libvirt error. + */ int -virFileReadAllQuiet(const char *path, int maxlen, char **buf) +virFileReadAllQuiet(const char *path, + int maxlen, + char **buf) { int fd; int len; @@ -1639,13 +1737,26 @@ virFileReadAllQuiet(const char *path, int maxlen, char **buf) return len; } -/* Read @file into preallocated buffer @buf of size @len. - * Return value is -errno in case of errors and size - * of data read (no trailing zero) in case of success. - * If there is more data then @len - 1 then data will be - * truncated. */ + +/** + * virFileReadBufQuiet: + * @file: file to read + * @buf: buffer to read files into + * @len: size of @buf + * + * Reads up to @len - 1 bytes of @file into @buf. + * + * The buffer @buf is guaranteed to be terminated with a NUL ('\0') byte one + * byte beyond the read contents of the file. The NUL byte is not included in + * the returned amount of bytes read. + * + * Returns number of bytes actually read from file on success, -errno on error + * and doesn't raise a libvirt error. + */ int -virFileReadBufQuiet(const char *file, char *buf, int len) +virFileReadBufQuiet(const char *file, + char *buf, + int len) { int fd; ssize_t sz; @@ -1663,6 +1774,7 @@ virFileReadBufQuiet(const char *file, char *buf, int len) return sz; } + /* Truncate @path and write @str to it. If @mode is 0, ensure that @path exists; otherwise, use @mode if @path must be created. Return 0 for success, nonzero for failure. -- 2.53.0

From: Peter Krempa <pkrempa@redhat.com> Limit the size of the4 allocated buffer to max_len + 1. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/util/virfile.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index bc3faedd4e..e7549197cd 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1503,11 +1503,14 @@ saferead_lim(int fd, size_t max_len, size_t *length) int count; int requested; - if (size + BUFSIZ + 1 > alloc) { + if (alloc < max_len + 1 && + size + BUFSIZ + 1 > alloc) { alloc += alloc / 2; if (alloc < size + BUFSIZ + 1) alloc = size + BUFSIZ + 1; + alloc = MIN(alloc, max_len + 1); + VIR_REALLOC_N(buf, alloc); } -- 2.53.0