[PATCH] virpci: Stop writing out of bounds in virPCIDeviceReadClass()
From: Michal Privoznik <mprivozn@redhat.com> Inside of virPCIDeviceReadClass() there's a call to virFileReadAll(). This reads contents of given file into a buffer (id_str). To make sure the buffer is NUL terminated string there's then write of NUL byte at 9th position of the buffer. Well, this is redundant as virFileReadAll() made sure the buffer is properly terminated on success (transitively¸ via saferead_lim()). But it is also wrong, because there's no guarantee the file is more than 8 bytes long. Just remove the NUL termination and rely on virFileReadAll() to properly terminate the buffer. Signed-off-by: Michal Privoznik <mprivozn@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.52.0
On Fri, Mar 27, 2026 at 10:13:26 +0100, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
Inside of virPCIDeviceReadClass() there's a call to virFileReadAll(). This reads contents of given file into a buffer (id_str). To make sure the buffer is NUL terminated string there's then write of NUL byte at 9th position of the buffer. Well, this is redundant as virFileReadAll() made sure the buffer is properly terminated on success (transitively¸ via saferead_lim()). But it is also wrong, because there's no guarantee the file is more than 8 bytes long.
I've just posted an more extensive series, which also documents virFileReadAll to prevent such misunderstandings, fixes one more instance of the same problem which didn't yet manifest itself because the overwritten byte was exactly the NUL terminator, and removes few other redundant attempts to NUL-out the end of the buffer from virFileReadAll and friends.
Just remove the NUL termination and rely on virFileReadAll() to properly terminate the buffer.
If you want to push this now you can: Reviewed-by: Peter Krempa <pkrempa@redhat.com>
On 3/27/26 10:57, Peter Krempa wrote:
On Fri, Mar 27, 2026 at 10:13:26 +0100, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
Inside of virPCIDeviceReadClass() there's a call to virFileReadAll(). This reads contents of given file into a buffer (id_str). To make sure the buffer is NUL terminated string there's then write of NUL byte at 9th position of the buffer. Well, this is redundant as virFileReadAll() made sure the buffer is properly terminated on success (transitively¸ via saferead_lim()). But it is also wrong, because there's no guarantee the file is more than 8 bytes long.
I've just posted an more extensive series, which also documents virFileReadAll to prevent such misunderstandings, fixes one more instance of the same problem which didn't yet manifest itself because the overwritten byte was exactly the NUL terminator, and removes few other redundant attempts to NUL-out the end of the buffer from virFileReadAll and friends.
Just remove the NUL termination and rely on virFileReadAll() to properly terminate the buffer.
If you want to push this now you can:
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
Nah, yours is more extensive so lets go with your patches. Michal
participants (3)
-
Michal Privoznik -
Michal Prívozník
-
Peter Krempa