Re: [libvirt] [PATCH] node_device_driver.c: don't write beyond EOB for 4K-byte symlink

[Jim Meyering <jim@xxxxxxxxxxxx>]

Jim Meyering wrote:
> Without this patch, a symlink pointing to a 4096-byte name
> could make this code write NUL into the byte beyond end of buffer:
>
>     if ((n = readlink(driver_link, devpath, sizeof devpath - 1)) < 0) {
>         virReportSystemError(conn, errno,
>                              _("cannot resolve driver link %s"), driver_link);
>         goto cleanup;
>     }
>     devpath[n] = '\0';
>
>
>>From a075e207bc8fb279c43c9f4f43a960ffbd9a8a70 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering@xxxxxxxxxx>
> Date: Mon, 14 Dec 2009 12:05:38 +0100
> Subject: [PATCH] node_device_driver.c: don't write beyond EOB for 4K-byte symlink
>
> * src/node_device/node_device_driver.c (update_driver_name): Leave
> one byte for the trailing NUL we'll append.
> ---
>  src/node_device/node_device_driver.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
> index f083f16..eda5d5e 100644
> --- a/src/node_device/node_device_driver.c
> +++ b/src/node_device/node_device_driver.c
> @@ -97,7 +97,7 @@ static int update_driver_name(virConnectPtr conn,
>          goto cleanup;
>      }
>
> -    if ((n = readlink(driver_link, devpath, sizeof devpath)) < 0) {
> +    if ((n = readlink(driver_link, devpath, sizeof devpath - 1)) < 0) {
>          virReportSystemError(conn, errno,
>                               _("cannot resolve driver link %s"), driver_link);
>          goto cleanup;

The above is correct, but Daniel Veillard suggested a better (albeit
slightly larger) change: use virFileResolveLink instead of readlink:

>From 4ae050481d481629fc98e8e7f5322ce6d724d3f7 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering@xxxxxxxxxx>
Date: Mon, 14 Dec 2009 12:05:38 +0100
Subject: [PATCH] node_device_driver.c: don't write beyond EOB for 4K-byte symlink

* src/node_device/node_device_driver.c (update_driver_name): The
previous code would write one byte beyond the end of the 4KiB
stack buffer when presented with a symlink value of exactly that
length (very unlikely).  Remove the automatic buffer and use
virFileResolveLink in place of readlink.  Suggested by Daniel Veillard.
---
 src/node_device/node_device_driver.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index f083f16..ecbac0f 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -78,10 +78,9 @@ static int update_driver_name(virConnectPtr conn,
                               virNodeDeviceObjPtr dev)
 {
     char *driver_link = NULL;
-    char devpath[PATH_MAX];
+    char *devpath;
     char *p;
     int ret = -1;
-    int n;

     VIR_FREE(dev->def->driver);

@@ -97,12 +96,11 @@ static int update_driver_name(virConnectPtr conn,
         goto cleanup;
     }

-    if ((n = readlink(driver_link, devpath, sizeof devpath)) < 0) {
+    if (virFileResolveLink(driver_link, &devpath) < 0) {
         virReportSystemError(conn, errno,
                              _("cannot resolve driver link %s"), driver_link);
         goto cleanup;
     }
-    devpath[n] = '\0';

     p = strrchr(devpath, '/');
     if (p) {
@@ -116,6 +114,7 @@ static int update_driver_name(virConnectPtr conn,

 cleanup:
     VIR_FREE(driver_link);
+    free(devpath);
     return ret;
 }
 #else
--
1.6.6.rc2.275.g51e2d

--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list