DPDK patches and discussions
 help / color / mirror / Atom feed
* [PATCH v2 0/2] Vhost: fix FD leaks and improve logs
@ 2023-01-27 16:55 Maxime Coquelin
  2023-01-27 16:55 ` [PATCH v2 1/2] vhost: fix possible FDs leak Maxime Coquelin
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Maxime Coquelin @ 2023-01-27 16:55 UTC (permalink / raw)
  To: dev, david.marchand, chenbo.xia; +Cc: 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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 1/2] vhost: fix possible FDs leak
  2023-01-27 16:55 [PATCH v2 0/2] Vhost: fix FD leaks and improve logs Maxime Coquelin
@ 2023-01-27 16:55 ` Maxime Coquelin
  2023-01-29  9:25   ` David Marchand
  2023-02-07  5:38   ` Xia, Chenbo
  2023-01-27 16:55 ` [PATCH v2 2/2] vhost: fix possible FD leaks on MSG_TRUNC and MSG_CTRUNC Maxime Coquelin
  2023-01-27 16:55 ` [PATCH v2 2/2] vhost: fix possible FD leaks on truncation Maxime Coquelin
  2 siblings, 2 replies; 11+ messages in thread
From: Maxime Coquelin @ 2023-01-27 16:55 UTC (permalink / raw)
  To: dev, david.marchand, chenbo.xia; +Cc: Maxime Coquelin, stable

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 2/2] vhost: fix possible FD leaks on MSG_TRUNC and MSG_CTRUNC
  2023-01-27 16:55 [PATCH v2 0/2] Vhost: fix FD leaks and improve logs Maxime Coquelin
  2023-01-27 16:55 ` [PATCH v2 1/2] vhost: fix possible FDs leak Maxime Coquelin
@ 2023-01-27 16:55 ` Maxime Coquelin
  2023-01-29  9:26   ` David Marchand
  2023-01-27 16:55 ` [PATCH v2 2/2] vhost: fix possible FD leaks on truncation Maxime Coquelin
  2 siblings, 1 reply; 11+ messages in thread
From: Maxime Coquelin @ 2023-01-27 16:55 UTC (permalink / raw)
  To: dev, david.marchand, chenbo.xia; +Cc: Maxime Coquelin, stable

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2 2/2] vhost: fix possible FD leaks on truncation
  2023-01-27 16:55 [PATCH v2 0/2] Vhost: fix FD leaks and improve logs Maxime Coquelin
  2023-01-27 16:55 ` [PATCH v2 1/2] vhost: fix possible FDs leak Maxime Coquelin
  2023-01-27 16:55 ` [PATCH v2 2/2] vhost: fix possible FD leaks on MSG_TRUNC and MSG_CTRUNC Maxime Coquelin
@ 2023-01-27 16:55 ` Maxime Coquelin
  2023-02-07  5:38   ` Xia, Chenbo
  2 siblings, 1 reply; 11+ messages in thread
From: Maxime Coquelin @ 2023-01-27 16:55 UTC (permalink / raw)
  To: dev, david.marchand, chenbo.xia; +Cc: Maxime Coquelin, stable

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/2] vhost: fix possible FDs leak
  2023-01-27 16:55 ` [PATCH v2 1/2] vhost: fix possible FDs leak Maxime Coquelin
@ 2023-01-29  9:25   ` David Marchand
  2023-01-30  9:46     ` Maxime Coquelin
  2023-02-07  5:38   ` Xia, Chenbo
  1 sibling, 1 reply; 11+ messages in thread
From: David Marchand @ 2023-01-29  9:25 UTC (permalink / raw)
  To: Maxime Coquelin; +Cc: dev, chenbo.xia, stable

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 2/2] vhost: fix possible FD leaks on MSG_TRUNC and MSG_CTRUNC
  2023-01-27 16:55 ` [PATCH v2 2/2] vhost: fix possible FD leaks on MSG_TRUNC and MSG_CTRUNC Maxime Coquelin
@ 2023-01-29  9:26   ` David Marchand
  0 siblings, 0 replies; 11+ messages in thread
From: David Marchand @ 2023-01-29  9:26 UTC (permalink / raw)
  To: Maxime Coquelin; +Cc: dev, chenbo.xia, stable

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/2] vhost: fix possible FDs leak
  2023-01-29  9:25   ` David Marchand
@ 2023-01-30  9:46     ` Maxime Coquelin
  2023-01-30 14:25       ` David Marchand
  0 siblings, 1 reply; 11+ messages in thread
From: Maxime Coquelin @ 2023-01-30  9:46 UTC (permalink / raw)
  To: David Marchand; +Cc: dev, chenbo.xia, stable



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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/2] vhost: fix possible FDs leak
  2023-01-30  9:46     ` Maxime Coquelin
@ 2023-01-30 14:25       ` David Marchand
  2023-02-07 16:18         ` Maxime Coquelin
  0 siblings, 1 reply; 11+ messages in thread
From: David Marchand @ 2023-01-30 14:25 UTC (permalink / raw)
  To: Maxime Coquelin; +Cc: dev, chenbo.xia, stable

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH v2 1/2] vhost: fix possible FDs leak
  2023-01-27 16:55 ` [PATCH v2 1/2] vhost: fix possible FDs leak Maxime Coquelin
  2023-01-29  9:25   ` David Marchand
@ 2023-02-07  5:38   ` Xia, Chenbo
  1 sibling, 0 replies; 11+ messages in thread
From: Xia, Chenbo @ 2023-02-07  5:38 UTC (permalink / raw)
  To: Maxime Coquelin, dev, david.marchand; +Cc: Coquelin, Maxime, stable


> -----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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH v2 2/2] vhost: fix possible FD leaks on truncation
  2023-01-27 16:55 ` [PATCH v2 2/2] vhost: fix possible FD leaks on truncation Maxime Coquelin
@ 2023-02-07  5:38   ` Xia, Chenbo
  0 siblings, 0 replies; 11+ messages in thread
From: Xia, Chenbo @ 2023-02-07  5:38 UTC (permalink / raw)
  To: Maxime Coquelin, dev, david.marchand; +Cc: Coquelin, Maxime, stable

> -----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> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2 1/2] vhost: fix possible FDs leak
  2023-01-30 14:25       ` David Marchand
@ 2023-02-07 16:18         ` Maxime Coquelin
  0 siblings, 0 replies; 11+ messages in thread
From: Maxime Coquelin @ 2023-02-07 16:18 UTC (permalink / raw)
  To: David Marchand; +Cc: dev, chenbo.xia, stable



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


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2023-02-07 16:18 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-27 16:55 [PATCH v2 0/2] Vhost: fix FD leaks and improve logs Maxime Coquelin
2023-01-27 16:55 ` [PATCH v2 1/2] vhost: fix possible FDs leak Maxime Coquelin
2023-01-29  9:25   ` David Marchand
2023-01-30  9:46     ` Maxime Coquelin
2023-01-30 14:25       ` David Marchand
2023-02-07 16:18         ` Maxime Coquelin
2023-02-07  5:38   ` Xia, Chenbo
2023-01-27 16:55 ` [PATCH v2 2/2] vhost: fix possible FD leaks on MSG_TRUNC and MSG_CTRUNC Maxime Coquelin
2023-01-29  9:26   ` David Marchand
2023-01-27 16:55 ` [PATCH v2 2/2] vhost: fix possible FD leaks on truncation Maxime Coquelin
2023-02-07  5:38   ` Xia, Chenbo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).