vfio-user: validate VERSION replies

[PATCH] vfio-user: validate VERSION replies

Posted by zhaoguohan@kylinos.cn 1 month, 1 week ago

From: GuoHan Zhao <zhaoguohan@kylinos.cn>

The vfio-user protocol makes the VERSION payload optional, so a
reply may legally stop after the major and minor fields.

vfio_user_validate_version() currently assumes a capabilities string is
always present and NUL-terminated. When the server replies without
version data, QEMU ends up reusing the request-side capabilities buffer
and the terminating-NUL check underflows. Replies shorter than the fixed
VERSION header are also accessed before they are validated.

Reject replies shorter than the fixed VERSION header and only parse
capabilities when the reply actually carries version data.

Fixes: 36227628d824 (vfio-user: implement message send infrastructure)
Signed-off-by: GuoHan Zhao <zhaoguohan@kylinos.cn>
---
 hw/vfio-user/proxy.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/hw/vfio-user/proxy.c b/hw/vfio-user/proxy.c
index 0f7d8425d614..197aee07bf7a 100644
--- a/hw/vfio-user/proxy.c
+++ b/hw/vfio-user/proxy.c
@@ -1292,7 +1292,7 @@ bool vfio_user_validate_version(VFIOUserProxy *proxy, Error **errp)
 {
     g_autofree VFIOUserVersion *msgp = NULL;
     GString *caps;
-    char *reply;
+    const char *reply = "";
     int size, caplen;
 
     caps = caps_json();
@@ -1322,17 +1322,24 @@ bool vfio_user_validate_version(VFIOUserProxy *proxy, Error **errp)
         return false;
     }
 
-    reply = msgp->capabilities;
-    if (reply[msgp->hdr.size - sizeof(*msgp) - 1] != '\0') {
-        error_setg(errp, "corrupt version reply");
+    if (msgp->hdr.size < sizeof(*msgp)) {
+        error_setg(errp, "short version reply");
         return false;
     }
 
-    if (!caps_check(proxy, msgp->minor, reply, errp)) {
-        return false;
+    if (msgp->hdr.size > sizeof(*msgp)) {
+        reply = msgp->capabilities;
+        if (reply[msgp->hdr.size - sizeof(*msgp) - 1] != '\0') {
+            error_setg(errp, "corrupt version reply");
+            return false;
+        }
+
+        if (!caps_check(proxy, msgp->minor, reply, errp)) {
+            return false;
+        }
     }
 
-    trace_vfio_user_version(msgp->major, msgp->minor, msgp->capabilities);
+    trace_vfio_user_version(msgp->major, msgp->minor, reply);
     return true;
 }
 
-- 
2.43.0

Re: [PATCH] vfio-user: validate VERSION replies

Posted by Cédric Le Goater 2 weeks, 2 days ago

On 6/3/26 08:21, zhaoguohan@kylinos.cn wrote:
> From: GuoHan Zhao <zhaoguohan@kylinos.cn>
> 
> The vfio-user protocol makes the VERSION payload optional, so a
> reply may legally stop after the major and minor fields.
> 
> vfio_user_validate_version() currently assumes a capabilities string is
> always present and NUL-terminated. When the server replies without
> version data, QEMU ends up reusing the request-side capabilities buffer
> and the terminating-NUL check underflows. Replies shorter than the fixed
> VERSION header are also accessed before they are validated.
> 
> Reject replies shorter than the fixed VERSION header and only parse
> capabilities when the reply actually carries version data.
> 
> Fixes: 36227628d824 (vfio-user: implement message send infrastructure)
> Signed-off-by: GuoHan Zhao <zhaoguohan@kylinos.cn>
> ---
>   hw/vfio-user/proxy.c | 21 ++++++++++++++-------
>   1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/vfio-user/proxy.c b/hw/vfio-user/proxy.c
> index 0f7d8425d614..197aee07bf7a 100644
> --- a/hw/vfio-user/proxy.c
> +++ b/hw/vfio-user/proxy.c
> @@ -1292,7 +1292,7 @@ bool vfio_user_validate_version(VFIOUserProxy *proxy, Error **errp)
>   {
>       g_autofree VFIOUserVersion *msgp = NULL;
>       GString *caps;
> -    char *reply;
> +    const char *reply = "";
>       int size, caplen;
>   
>       caps = caps_json();
> @@ -1322,17 +1322,24 @@ bool vfio_user_validate_version(VFIOUserProxy *proxy, Error **errp)
>           return false;
>       }
>   
> -    reply = msgp->capabilities;
> -    if (reply[msgp->hdr.size - sizeof(*msgp) - 1] != '\0') {
> -        error_setg(errp, "corrupt version reply");
> +    if (msgp->hdr.size < sizeof(*msgp)) {
> +        error_setg(errp, "short version reply");
>           return false;
>       }
>   
> -    if (!caps_check(proxy, msgp->minor, reply, errp)) {
> -        return false;
> +    if (msgp->hdr.size > sizeof(*msgp)) {
> +        reply = msgp->capabilities;
> +        if (reply[msgp->hdr.size - sizeof(*msgp) - 1] != '\0') {
> +            error_setg(errp, "corrupt version reply");
> +            return false;
> +        }
> +
> +        if (!caps_check(proxy, msgp->minor, reply, errp)) {
> +            return false;
> +        }
>       }
>   
> -    trace_vfio_user_version(msgp->major, msgp->minor, msgp->capabilities);
> +    trace_vfio_user_version(msgp->major, msgp->minor, reply);
>       return true;
>   }
>   


Applied to

     https://github.com/legoater/qemu vfio-next

Thanks,

C.

Re: [PATCH] vfio-user: validate VERSION replies

Posted by John Levon 3 weeks, 4 days ago

On Wed, Jun 03, 2026 at 02:21:38PM +0800, zhaoguohan@kylinos.cn wrote:

> From: GuoHan Zhao <zhaoguohan@kylinos.cn>
> 
> The vfio-user protocol makes the VERSION payload optional, so a
> reply may legally stop after the major and minor fields.
> 
> vfio_user_validate_version() currently assumes a capabilities string is
> always present and NUL-terminated. When the server replies without
> version data, QEMU ends up reusing the request-side capabilities buffer
> and the terminating-NUL check underflows. Replies shorter than the fixed
> VERSION header are also accessed before they are validated.
> 
> Reject replies shorter than the fixed VERSION header and only parse
> capabilities when the reply actually carries version data.
> 
> Fixes: 36227628d824 (vfio-user: implement message send infrastructure)
> Signed-off-by: GuoHan Zhao <zhaoguohan@kylinos.cn>

Thanks!

Reviewed-by: John Levon <john.levon@nutanix.com>

regards
john

Re: [PATCH] vfio-user: validate VERSION replies

Posted by Cédric Le Goater 1 month ago

John, Thanos,

Does this change look good to you ?

Thanks,

C.


On 6/3/26 08:21, zhaoguohan@kylinos.cn wrote:
> From: GuoHan Zhao <zhaoguohan@kylinos.cn>
> 
> The vfio-user protocol makes the VERSION payload optional, so a
> reply may legally stop after the major and minor fields.
> 
> vfio_user_validate_version() currently assumes a capabilities string is
> always present and NUL-terminated. When the server replies without
> version data, QEMU ends up reusing the request-side capabilities buffer
> and the terminating-NUL check underflows. Replies shorter than the fixed
> VERSION header are also accessed before they are validated.
> 
> Reject replies shorter than the fixed VERSION header and only parse
> capabilities when the reply actually carries version data.
> 
> Fixes: 36227628d824 (vfio-user: implement message send infrastructure)
> Signed-off-by: GuoHan Zhao <zhaoguohan@kylinos.cn>
> ---
>   hw/vfio-user/proxy.c | 21 ++++++++++++++-------
>   1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/vfio-user/proxy.c b/hw/vfio-user/proxy.c
> index 0f7d8425d614..197aee07bf7a 100644
> --- a/hw/vfio-user/proxy.c
> +++ b/hw/vfio-user/proxy.c
> @@ -1292,7 +1292,7 @@ bool vfio_user_validate_version(VFIOUserProxy *proxy, Error **errp)
>   {
>       g_autofree VFIOUserVersion *msgp = NULL;
>       GString *caps;
> -    char *reply;
> +    const char *reply = "";
>       int size, caplen;
>   
>       caps = caps_json();
> @@ -1322,17 +1322,24 @@ bool vfio_user_validate_version(VFIOUserProxy *proxy, Error **errp)
>           return false;
>       }
>   
> -    reply = msgp->capabilities;
> -    if (reply[msgp->hdr.size - sizeof(*msgp) - 1] != '\0') {
> -        error_setg(errp, "corrupt version reply");
> +    if (msgp->hdr.size < sizeof(*msgp)) {
> +        error_setg(errp, "short version reply");
>           return false;
>       }
>   
> -    if (!caps_check(proxy, msgp->minor, reply, errp)) {
> -        return false;
> +    if (msgp->hdr.size > sizeof(*msgp)) {
> +        reply = msgp->capabilities;
> +        if (reply[msgp->hdr.size - sizeof(*msgp) - 1] != '\0') {
> +            error_setg(errp, "corrupt version reply");
> +            return false;
> +        }
> +
> +        if (!caps_check(proxy, msgp->minor, reply, errp)) {
> +            return false;
> +        }
>       }
>   
> -    trace_vfio_user_version(msgp->major, msgp->minor, msgp->capabilities);
> +    trace_vfio_user_version(msgp->major, msgp->minor, reply);
>       return true;
>   }
>

Re: [PATCH] vfio-user: validate VERSION replies

Posted by John Levon 1 month ago

On Fri, Jun 12, 2026 at 11:00:35AM +0200, Cédric Le Goater wrote:

> John, Thanos,
> 
> Does this change look good to you ?

Sorry for the delay, it's in my inbox and I'll try to get to it soon

regards
john