So that padding or uninitialized bytes can't leak guest memory contents.
Signed-off-by: Andrea Parri (Microsoft) <[email protected]>
---
net/vmw_vsock/hyperv_transport.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
index 092cadc2c866d..72ce00928c8e7 100644
--- a/net/vmw_vsock/hyperv_transport.c
+++ b/net/vmw_vsock/hyperv_transport.c
@@ -655,7 +655,7 @@ static ssize_t hvs_stream_enqueue(struct vsock_sock *vsk, struct msghdr *msg,
BUILD_BUG_ON(sizeof(*send_buf) != HV_HYP_PAGE_SIZE);
- send_buf = kmalloc(sizeof(*send_buf), GFP_KERNEL);
+ send_buf = kzalloc(sizeof(*send_buf), GFP_KERNEL);
if (!send_buf)
return -ENOMEM;
--
2.25.1
> > @@ -655,7 +655,7 @@ static ssize_t hvs_stream_enqueue(struct vsock_sock *vsk,
> > struct msghdr *msg,
> >
> > BUILD_BUG_ON(sizeof(*send_buf) != HV_HYP_PAGE_SIZE);
> >
> > - send_buf = kmalloc(sizeof(*send_buf), GFP_KERNEL);
> > + send_buf = kzalloc(sizeof(*send_buf), GFP_KERNEL);
>
> Is this change really needed?
The idea was...
> All fields are explicitly initialized, and in the data
> array, only the populated bytes are copied to the ring buffer. There should not
> be any uninitialized values sent to the host. Zeroing the memory ahead of
> time certainly provides an extra protection (particularly against padding bytes,
> but there can't be any since the layout of the data is part of the protocol with
> Hyper-V).
Rather than keeping checking that...
> It is expensive protection to zero out 16K+ bytes every time we send
> out a small message.
Do this. ;-)
Will drop the patch.
Thanks,
Andrea
From: Andrea Parri <[email protected]> Sent: Thursday, April 14, 2022 11:51 PM
>
> > > @@ -655,7 +655,7 @@ static ssize_t hvs_stream_enqueue(struct vsock_sock *vsk,
> > > struct msghdr *msg,
> > >
> > > BUILD_BUG_ON(sizeof(*send_buf) != HV_HYP_PAGE_SIZE);
> > >
> > > - send_buf = kmalloc(sizeof(*send_buf), GFP_KERNEL);
> > > + send_buf = kzalloc(sizeof(*send_buf), GFP_KERNEL);
> >
> > Is this change really needed?
>
> The idea was...
>
>
> > All fields are explicitly initialized, and in the data
> > array, only the populated bytes are copied to the ring buffer. There should not
> > be any uninitialized values sent to the host. Zeroing the memory ahead of
> > time certainly provides an extra protection (particularly against padding bytes,
> > but there can't be any since the layout of the data is part of the protocol with
> > Hyper-V).
>
> Rather than keeping checking that...
The extra protection might be obtained by just zero'ing the header (i.e., the
bytes up to the 16 Kbyte data array). I don't have a strong preference either
way, so up to you.
Michael
>
>
> > It is expensive protection to zero out 16K+ bytes every time we send
> > out a small message.
>
> Do this. ;-)
>
> Will drop the patch.
>
> Thanks,
> Andrea
> > > All fields are explicitly initialized, and in the data
> > > array, only the populated bytes are copied to the ring buffer. There should not
> > > be any uninitialized values sent to the host. Zeroing the memory ahead of
> > > time certainly provides an extra protection (particularly against padding bytes,
> > > but there can't be any since the layout of the data is part of the protocol with
> > > Hyper-V).
> >
> > Rather than keeping checking that...
>
> The extra protection might be obtained by just zero'ing the header (i.e., the
> bytes up to the 16 Kbyte data array). I don't have a strong preference either
> way, so up to you.
A main reason behind this RFC is that I don't have either. IIUC, you're
suggesting something like (the compiled only):
diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
index 092cadc2c866d..200f12c432863 100644
--- a/net/vmw_vsock/hyperv_transport.c
+++ b/net/vmw_vsock/hyperv_transport.c
@@ -234,7 +234,8 @@ static int __hvs_send_data(struct vmbus_channel *chan,
{
hdr->pkt_type = 1;
hdr->data_size = to_write;
- return vmbus_sendpacket(chan, hdr, sizeof(*hdr) + to_write,
+ return vmbus_sendpacket(chan, hdr,
+ offsetof(struct hvs_send_buf, data) + to_write,
0, VM_PKT_DATA_INBAND, 0);
}
@@ -658,6 +659,7 @@ static ssize_t hvs_stream_enqueue(struct vsock_sock *vsk, struct msghdr *msg,
send_buf = kmalloc(sizeof(*send_buf), GFP_KERNEL);
if (!send_buf)
return -ENOMEM;
+ memset(send_buf, 0, offsetof(struct hvs_send_buf, data));
/* Reader(s) could be draining data from the channel as we write.
* Maximize bandwidth, by iterating until the channel is found to be
--
Let me queue this for further testing/review...
Thanks,
Andrea
From: Andrea Parri (Microsoft) <[email protected]> Sent: Wednesday, April 13, 2022 1:48 PM
>
> So that padding or uninitialized bytes can't leak guest memory contents.
>
> Signed-off-by: Andrea Parri (Microsoft) <[email protected]>
> ---
> net/vmw_vsock/hyperv_transport.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
> index 092cadc2c866d..72ce00928c8e7 100644
> --- a/net/vmw_vsock/hyperv_transport.c
> +++ b/net/vmw_vsock/hyperv_transport.c
> @@ -655,7 +655,7 @@ static ssize_t hvs_stream_enqueue(struct vsock_sock *vsk,
> struct msghdr *msg,
>
> BUILD_BUG_ON(sizeof(*send_buf) != HV_HYP_PAGE_SIZE);
>
> - send_buf = kmalloc(sizeof(*send_buf), GFP_KERNEL);
> + send_buf = kzalloc(sizeof(*send_buf), GFP_KERNEL);
Is this change really needed? All fields are explicitly initialized, and in the data
array, only the populated bytes are copied to the ring buffer. There should not
be any uninitialized values sent to the host. Zeroing the memory ahead of
time certainly provides an extra protection (particularly against padding bytes,
but there can't be any since the layout of the data is part of the protocol with
Hyper-V). It is expensive protection to zero out 16K+ bytes every time we send
out a small message.
Michael
> if (!send_buf)
> return -ENOMEM;
>
> --
> 2.25.1