On 1/30/23 15:25, David Marchand wrote:
On Mon, Jan 30, 2023 at 10:46 AM Maxime Coquelin
<maxime.coque...@redhat.com> wrote:
On 1/29/23 10:25, David Marchand wrote:
On Fri, Jan 27, 2023 at 5:55 PM Maxime Coquelin
<maxime.coque...@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: sta...@dpdk.org
Signed-off-by: Maxime Coquelin <maxime.coque...@redhat.com>
Reviewed-by: David Marchand <david.march...@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
