[PATCH v2 0/2] Vhost: fix FD leaks and improve logs

[PATCH v2 0/2] Vhost: fix FD leaks and improve logs

[Maxime Coquelin]

This two-patches series fix possible FD leaks when
receiving Vhost-user messages fails. It also improve
logging by differentiating message's buffer truncation
and control data truncation.

Maxime Coquelin (2):
  vhost: fix possible FDs leak
  vhost: fix possible FD leaks on truncation

 lib/vhost/socket.c     |  8 +++++---
 lib/vhost/vhost_user.c | 23 +++++++++++++++--------
 2 files changed, 20 insertions(+), 11 deletions(-)

-- 
2.39.1

[PATCH v2 1/2] vhost: fix possible FDs leak

[Maxime Coquelin]

On failure, read_vhost_message() only closed the message
FDs if the header size was unexpected, but there are other
cases where it is required. For exemple in the case the
payload size read from the header is greater than the
expected maximum payload size.

This patch fixes this by closing all messages FDs in all
error cases.

Fixes: bf472259dde6 ("vhost: fix possible denial of service by leaking FDs")
Cc: stable@dpdk.org

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/vhost_user.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
index 9902ae9944..943058725e 100644
--- a/lib/vhost/vhost_user.c
+++ b/lib/vhost/vhost_user.c
@@ -2817,29 +2817,36 @@ read_vhost_message(struct virtio_net *dev, int sockfd, struct  vhu_msg_context *
 
 	ret = read_fd_message(dev->ifname, sockfd, (char *)&ctx->msg, VHOST_USER_HDR_SIZE,
 		ctx->fds, VHOST_MEMORY_MAX_NREGIONS, &ctx->fd_num);
-	if (ret <= 0) {
-		return ret;
-	} else if (ret != VHOST_USER_HDR_SIZE) {
+	if (ret <= 0)
+		goto out;
+
+	if (ret != VHOST_USER_HDR_SIZE) {
 		VHOST_LOG_CONFIG(dev->ifname, ERR, "Unexpected header size read\n");
-		close_msg_fds(ctx);
-		return -1;
+		ret = -1;
+		goto out;
 	}
 
 	if (ctx->msg.size) {
 		if (ctx->msg.size > sizeof(ctx->msg.payload)) {
 			VHOST_LOG_CONFIG(dev->ifname, ERR, "invalid msg size: %d\n",
 				ctx->msg.size);
-			return -1;
+			ret = -1;
+			goto out;
 		}
 		ret = read(sockfd, &ctx->msg.payload, ctx->msg.size);
 		if (ret <= 0)
-			return ret;
+			goto out;
 		if (ret != (int)ctx->msg.size) {
 			VHOST_LOG_CONFIG(dev->ifname, ERR, "read control message failed\n");
-			return -1;
+			ret = -1;
+			goto out;
 		}
 	}
 
+out:
+	if (ret <= 0)
+		close_msg_fds(ctx);
+
 	return ret;
 }
 
-- 
2.39.1

[PATCH v2 2/2] vhost: fix possible FD leaks on MSG_TRUNC and MSG_CTRUNC

[Maxime Coquelin]

This patch fixes possible FDs leaks when truncation happens
on either the message buffer or its control data. Indeed,
by returning early, it did not let a chance to retrieve the
FDs passed as ancillary data, and so caused a potential FDs
leak.

This patch fixes this by extracting the FDs from the
ancillary data as long as recvmsg() call succeeded. It also
improves the logs to differentiate between MSG_TRUNC and
MSG_CTRUNC.

Fixes: bf472259dde6 ("vhost: fix possible denial of service by leaking FDs")
Cc: stable@dpdk.org

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/socket.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lib/vhost/socket.c b/lib/vhost/socket.c
index 863a6f6d52..669c322e12 100644
--- a/lib/vhost/socket.c
+++ b/lib/vhost/socket.c
@@ -129,10 +129,12 @@ read_fd_message(char *ifname, int sockfd, char *buf, int buflen, int *fds, int m
 		return ret;
 	}
 
-	if (msgh.msg_flags & (MSG_TRUNC | MSG_CTRUNC)) {
+	if (msgh.msg_flags & MSG_TRUNC)
 		VHOST_LOG_CONFIG(ifname, ERR, "truncated msg (fd %d)\n", sockfd);
-		return -1;
-	}
+
+	/* MSG_CTRUNC may be caused by LSM misconfiguration */
+	if (msgh.msg_flags & MSG_CTRUNC)
+		VHOST_LOG_CONFIG(ifname, ERR, "truncated control data (fd %d)\n", sockfd);
 
 	for (cmsg = CMSG_FIRSTHDR(&msgh); cmsg != NULL;
 		cmsg = CMSG_NXTHDR(&msgh, cmsg)) {
-- 
2.39.1

[PATCH v2 2/2] vhost: fix possible FD leaks on truncation

[Maxime Coquelin]

This patch fixes possible FDs leaks when truncation happens
on either the message buffer or its control data. Indeed,
by returning early, it did not let a chance to retrieve the
FDs passed as ancillary data, and so caused a potential FDs
leak.

This patch fixes this by extracting the FDs from the
ancillary data as long as recvmsg() call succeeded. It also
improves the logs to differentiate between MSG_TRUNC and
MSG_CTRUNC.

Fixes: bf472259dde6 ("vhost: fix possible denial of service by leaking FDs")
Cc: stable@dpdk.org

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/socket.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lib/vhost/socket.c b/lib/vhost/socket.c
index 863a6f6d52..669c322e12 100644
--- a/lib/vhost/socket.c
+++ b/lib/vhost/socket.c
@@ -129,10 +129,12 @@ read_fd_message(char *ifname, int sockfd, char *buf, int buflen, int *fds, int m
 		return ret;
 	}
 
-	if (msgh.msg_flags & (MSG_TRUNC | MSG_CTRUNC)) {
+	if (msgh.msg_flags & MSG_TRUNC)
 		VHOST_LOG_CONFIG(ifname, ERR, "truncated msg (fd %d)\n", sockfd);
-		return -1;
-	}
+
+	/* MSG_CTRUNC may be caused by LSM misconfiguration */
+	if (msgh.msg_flags & MSG_CTRUNC)
+		VHOST_LOG_CONFIG(ifname, ERR, "truncated control data (fd %d)\n", sockfd);
 
 	for (cmsg = CMSG_FIRSTHDR(&msgh); cmsg != NULL;
 		cmsg = CMSG_NXTHDR(&msgh, cmsg)) {
-- 
2.39.1

Re: [PATCH v2 1/2] vhost: fix possible FDs leak

[David Marchand]

On Fri, Jan 27, 2023 at 5:55 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> On failure, read_vhost_message() only closed the message
> FDs if the header size was unexpected, but there are other
> cases where it is required. For exemple in the case the
> payload size read from the header is greater than the
> expected maximum payload size.
>
> This patch fixes this by closing all messages FDs in all
> error cases.
>
> Fixes: bf472259dde6 ("vhost: fix possible denial of service by leaking FDs")
> Cc: stable@dpdk.org
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Reviewed-by: David Marchand <david.marchand@redhat.com>

We mentionned offlist that the request type can be logged to help with debug.
Do you intend to add this as a follow up patch?


-- 
David Marchand

Re: [PATCH v2 2/2] vhost: fix possible FD leaks on MSG_TRUNC and MSG_CTRUNC

[David Marchand]

On Fri, Jan 27, 2023 at 5:55 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> This patch fixes possible FDs leaks when truncation happens
> on either the message buffer or its control data. Indeed,
> by returning early, it did not let a chance to retrieve the
> FDs passed as ancillary data, and so caused a potential FDs
> leak.
>
> This patch fixes this by extracting the FDs from the
> ancillary data as long as recvmsg() call succeeded. It also
> improves the logs to differentiate between MSG_TRUNC and
> MSG_CTRUNC.

As I mentionned offlist, I am not convinced the MSG_TRUNC flag can be
set on receipt of a message, since the socket is in stream mode.
I am okay to keep the check as is, but it is confusing.


>
> Fixes: bf472259dde6 ("vhost: fix possible denial of service by leaking FDs")
> Cc: stable@dpdk.org
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Reviewed-by: David Marchand <david.marchand@redhat.com>


-- 
David Marchand

Re: [PATCH v2 1/2] vhost: fix possible FDs leak

[Maxime Coquelin]

On 1/29/23 10:25, David Marchand wrote:
> On Fri, Jan 27, 2023 at 5:55 PM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>>
>> On failure, read_vhost_message() only closed the message
>> FDs if the header size was unexpected, but there are other
>> cases where it is required. For exemple in the case the
>> payload size read from the header is greater than the
>> expected maximum payload size.
>>
>> This patch fixes this by closing all messages FDs in all
>> error cases.
>>
>> Fixes: bf472259dde6 ("vhost: fix possible denial of service by leaking FDs")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> Reviewed-by: David Marchand <david.marchand@redhat.com>
> 
> We mentionned offlist that the request type can be logged to help with debug.
> Do you intend to add this as a follow up patch?
> 
> 

Thinking about it, that's not that trivial because read_vhost_message()
is called for both master and slave channels, so we would need to
differentiate between both to print proper request name.

It is doable by comparing the file descriptor passed as parameter with
the slave one stored in struct virtio_net, but that's not super clean.

Any thoughts?

Maxime

Re: [PATCH v2 1/2] vhost: fix possible FDs leak

[David Marchand]

On Mon, Jan 30, 2023 at 10:46 AM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
>
>
> On 1/29/23 10:25, David Marchand wrote:
> > On Fri, Jan 27, 2023 at 5:55 PM Maxime Coquelin
> > <maxime.coquelin@redhat.com> wrote:
> >>
> >> On failure, read_vhost_message() only closed the message
> >> FDs if the header size was unexpected, but there are other
> >> cases where it is required. For exemple in the case the
> >> payload size read from the header is greater than the
> >> expected maximum payload size.
> >>
> >> This patch fixes this by closing all messages FDs in all
> >> error cases.
> >>
> >> Fixes: bf472259dde6 ("vhost: fix possible denial of service by leaking FDs")
> >> Cc: stable@dpdk.org
> >>
> >> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >
> > Reviewed-by: David Marchand <david.marchand@redhat.com>
> >
> > We mentionned offlist that the request type can be logged to help with debug.
> > Do you intend to add this as a follow up patch?
> >
> >
>
> Thinking about it, that's not that trivial because read_vhost_message()
> is called for both master and slave channels, so we would need to
> differentiate between both to print proper request name.
>
> It is doable by comparing the file descriptor passed as parameter with
> the slave one stored in struct virtio_net, but that's not super clean.

From the 3 different callers of read_vhost_message, I think the only
one that could gain from this debug is vhost_user_msg_handler().

And here, we could look at the message content on read_vhost_message return.
Like this untested and ugly snippet:

diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
index 943058725e..1556f21a58 100644
--- a/lib/vhost/vhost_user.c
+++ b/lib/vhost/vhost_user.c
@@ -2994,13 +2994,10 @@ vhost_user_msg_handler(int vid, int fd)
                }
        }

+       ctx.msg.request.master = VHOST_USER_NONE;
        ret = read_vhost_message(dev, fd, &ctx);
-       if (ret <= 0) {
-               if (ret < 0)
-                       VHOST_LOG_CONFIG(dev->ifname, ERR, "vhost read
message failed\n");
-               else
-                       VHOST_LOG_CONFIG(dev->ifname, INFO, "vhost
peer closed\n");
-
+       if (ret == 0) {
+               VHOST_LOG_CONFIG(dev->ifname, INFO, "vhost peer closed\n");
                return -1;
        }

@@ -3010,6 +3007,14 @@ vhost_user_msg_handler(int vid, int fd)
        else
                msg_handler = NULL;

+       if (ret < 0) {
+               VHOST_LOG_CONFIG(dev->ifname, ERR, "vhost read message
%s%s%sfailed\n",
+                       msg_handler != NULL ? "for " : "",
+                       msg_handler != NULL ? msg_handler->description : "",
+                       msg_handler != NULL ? " " : "");
+               return -1;
+       }
+
        if (msg_handler != NULL && msg_handler->description != NULL) {
                if (request != VHOST_USER_IOTLB_MSG)
                        VHOST_LOG_CONFIG(dev->ifname, INFO,


-- 
David Marchand

RE: [PATCH v2 1/2] vhost: fix possible FDs leak

[Xia, Chenbo]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Saturday, January 28, 2023 12:56 AM
> To: dev@dpdk.org; david.marchand@redhat.com; Xia, Chenbo
> <chenbo.xia@intel.com>
> Cc: Coquelin, Maxime <maxime.coquelin@redhat.com>; stable@dpdk.org
> Subject: [PATCH v2 1/2] vhost: fix possible FDs leak
> 
> On failure, read_vhost_message() only closed the message
> FDs if the header size was unexpected, but there are other
> cases where it is required. For exemple in the case the

example

With this fixed:

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com> 

> payload size read from the header is greater than the
> expected maximum payload size.
> 
> This patch fixes this by closing all messages FDs in all
> error cases.
> 
> Fixes: bf472259dde6 ("vhost: fix possible denial of service by leaking
> FDs")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/vhost/vhost_user.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
> index 9902ae9944..943058725e 100644
> --- a/lib/vhost/vhost_user.c
> +++ b/lib/vhost/vhost_user.c
> @@ -2817,29 +2817,36 @@ read_vhost_message(struct virtio_net *dev, int
> sockfd, struct  vhu_msg_context *
> 
>  	ret = read_fd_message(dev->ifname, sockfd, (char *)&ctx->msg,
> VHOST_USER_HDR_SIZE,
>  		ctx->fds, VHOST_MEMORY_MAX_NREGIONS, &ctx->fd_num);
> -	if (ret <= 0) {
> -		return ret;
> -	} else if (ret != VHOST_USER_HDR_SIZE) {
> +	if (ret <= 0)
> +		goto out;
> +
> +	if (ret != VHOST_USER_HDR_SIZE) {
>  		VHOST_LOG_CONFIG(dev->ifname, ERR, "Unexpected header size
> read\n");
> -		close_msg_fds(ctx);
> -		return -1;
> +		ret = -1;
> +		goto out;
>  	}
> 
>  	if (ctx->msg.size) {
>  		if (ctx->msg.size > sizeof(ctx->msg.payload)) {
>  			VHOST_LOG_CONFIG(dev->ifname, ERR, "invalid msg
> size: %d\n",
>  				ctx->msg.size);
> -			return -1;
> +			ret = -1;
> +			goto out;
>  		}
>  		ret = read(sockfd, &ctx->msg.payload, ctx->msg.size);
>  		if (ret <= 0)
> -			return ret;
> +			goto out;
>  		if (ret != (int)ctx->msg.size) {
>  			VHOST_LOG_CONFIG(dev->ifname, ERR, "read control message
> failed\n");
> -			return -1;
> +			ret = -1;
> +			goto out;
>  		}
>  	}
> 
> +out:
> +	if (ret <= 0)
> +		close_msg_fds(ctx);
> +
>  	return ret;
>  }
> 
> --
> 2.39.1

RE: [PATCH v2 2/2] vhost: fix possible FD leaks on truncation

[Xia, Chenbo]

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Saturday, January 28, 2023 12:56 AM
> To: dev@dpdk.org; david.marchand@redhat.com; Xia, Chenbo
> <chenbo.xia@intel.com>
> Cc: Coquelin, Maxime <maxime.coquelin@redhat.com>; stable@dpdk.org
> Subject: [PATCH v2 2/2] vhost: fix possible FD leaks on truncation
> 
> This patch fixes possible FDs leaks when truncation happens
> on either the message buffer or its control data. Indeed,
> by returning early, it did not let a chance to retrieve the
> FDs passed as ancillary data, and so caused a potential FDs
> leak.
> 
> This patch fixes this by extracting the FDs from the
> ancillary data as long as recvmsg() call succeeded. It also
> improves the logs to differentiate between MSG_TRUNC and
> MSG_CTRUNC.
> 
> Fixes: bf472259dde6 ("vhost: fix possible denial of service by leaking
> FDs")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/vhost/socket.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/vhost/socket.c b/lib/vhost/socket.c
> index 863a6f6d52..669c322e12 100644
> --- a/lib/vhost/socket.c
> +++ b/lib/vhost/socket.c
> @@ -129,10 +129,12 @@ read_fd_message(char *ifname, int sockfd, char *buf,
> int buflen, int *fds, int m
>  		return ret;
>  	}
> 
> -	if (msgh.msg_flags & (MSG_TRUNC | MSG_CTRUNC)) {
> +	if (msgh.msg_flags & MSG_TRUNC)
>  		VHOST_LOG_CONFIG(ifname, ERR, "truncated msg (fd %d)\n",
> sockfd);
> -		return -1;
> -	}
> +
> +	/* MSG_CTRUNC may be caused by LSM misconfiguration */
> +	if (msgh.msg_flags & MSG_CTRUNC)
> +		VHOST_LOG_CONFIG(ifname, ERR, "truncated control data
> (fd %d)\n", sockfd);
> 
>  	for (cmsg = CMSG_FIRSTHDR(&msgh); cmsg != NULL;
>  		cmsg = CMSG_NXTHDR(&msgh, cmsg)) {
> --
> 2.39.1

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com> 

Re: [PATCH v2 1/2] vhost: fix possible FDs leak

[Maxime Coquelin]

On 1/30/23 15:25, David Marchand wrote:
> On Mon, Jan 30, 2023 at 10:46 AM Maxime Coquelin
> <maxime.coquelin@redhat.com> wrote:
>>
>>
>>
>> On 1/29/23 10:25, David Marchand wrote:
>>> On Fri, Jan 27, 2023 at 5:55 PM Maxime Coquelin
>>> <maxime.coquelin@redhat.com> wrote:
>>>>
>>>> On failure, read_vhost_message() only closed the message
>>>> FDs if the header size was unexpected, but there are other
>>>> cases where it is required. For exemple in the case the
>>>> payload size read from the header is greater than the
>>>> expected maximum payload size.
>>>>
>>>> This patch fixes this by closing all messages FDs in all
>>>> error cases.
>>>>
>>>> Fixes: bf472259dde6 ("vhost: fix possible denial of service by leaking FDs")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>
>>> Reviewed-by: David Marchand <david.marchand@redhat.com>
>>>
>>> We mentionned offlist that the request type can be logged to help with debug.
>>> Do you intend to add this as a follow up patch?
>>>
>>>
>>
>> Thinking about it, that's not that trivial because read_vhost_message()
>> is called for both master and slave channels, so we would need to
>> differentiate between both to print proper request name.
>>
>> It is doable by comparing the file descriptor passed as parameter with
>> the slave one stored in struct virtio_net, but that's not super clean.
> 
>  From the 3 different callers of read_vhost_message, I think the only
> one that could gain from this debug is vhost_user_msg_handler().
> 
> And here, we could look at the message content on read_vhost_message return.
> Like this untested and ugly snippet:
> 
> diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
> index 943058725e..1556f21a58 100644
> --- a/lib/vhost/vhost_user.c
> +++ b/lib/vhost/vhost_user.c
> @@ -2994,13 +2994,10 @@ vhost_user_msg_handler(int vid, int fd)
>                  }
>          }
> 
> +       ctx.msg.request.master = VHOST_USER_NONE;
>          ret = read_vhost_message(dev, fd, &ctx);
> -       if (ret <= 0) {
> -               if (ret < 0)
> -                       VHOST_LOG_CONFIG(dev->ifname, ERR, "vhost read
> message failed\n");
> -               else
> -                       VHOST_LOG_CONFIG(dev->ifname, INFO, "vhost
> peer closed\n");
> -
> +       if (ret == 0) {
> +               VHOST_LOG_CONFIG(dev->ifname, INFO, "vhost peer closed\n");
>                  return -1;
>          }
> 
> @@ -3010,6 +3007,14 @@ vhost_user_msg_handler(int vid, int fd)
>          else
>                  msg_handler = NULL;
> 
> +       if (ret < 0) {
> +               VHOST_LOG_CONFIG(dev->ifname, ERR, "vhost read message
> %s%s%sfailed\n",
> +                       msg_handler != NULL ? "for " : "",
> +                       msg_handler != NULL ? msg_handler->description : "",
> +                       msg_handler != NULL ? " " : "");
> +               return -1;
> +       }
> +
>          if (msg_handler != NULL && msg_handler->description != NULL) {
>                  if (request != VHOST_USER_IOTLB_MSG)
>                          VHOST_LOG_CONFIG(dev->ifname, INFO,
> 
> 

It looks good to me. I'm squashing this with patch 1, adding you SoB.

Thanks,
Maxime