diff mbox series

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

Message ID 20230127165540.37863-2-maxime.coquelin@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series Vhost: fix FD leaks and improve logs |

Checks

Context Check Description
ci/checkpatch warning coding style issues

Commit Message

Maxime Coquelin Jan. 27, 2023, 4:55 p.m. UTC 
  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(-)

Comments

David Marchand Jan. 29, 2023, 9:25 a.m. UTC | #1 
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?
Maxime Coquelin Jan. 30, 2023, 9:46 a.m. UTC | #2 
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
David Marchand Jan. 30, 2023, 2:25 p.m. UTC | #3 
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,
Chenbo Xia Feb. 7, 2023, 5:38 a.m. UTC | #4 
> -----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
Maxime Coquelin Feb. 7, 2023, 4:18 p.m. UTC | #5 
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
diff mbox series

Patch

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