The function returns NULL if the ring buffer has no enough space
available for a packet descriptor. The ring buffer's write_index
is in memory which is shared with the Hyper-V host, its value is
thus subject to being changed at any time.
Signed-off-by: Andrea Parri (Microsoft) <[email protected]>
---
net/vmw_vsock/hyperv_transport.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
index e111e13b66604..943352530936e 100644
--- a/net/vmw_vsock/hyperv_transport.c
+++ b/net/vmw_vsock/hyperv_transport.c
@@ -603,6 +603,8 @@ static ssize_t hvs_stream_dequeue(struct vsock_sock *vsk, struct msghdr *msg,
if (need_refill) {
hvs->recv_desc = hv_pkt_iter_first_raw(hvs->chan);
+ if (!hvs->recv_desc)
+ return -ENOBUFS;
ret = hvs_update_recv_data(hvs);
if (ret)
return ret;
--
2.25.1
On Fri, Apr 15, 2022 at 03:33:23AM +0000, Michael Kelley (LINUX) wrote:
> From: Andrea Parri (Microsoft) <[email protected]> Sent: Wednesday, April 13, 2022 1:48 PM
> >
> > The function returns NULL if the ring buffer has no enough space
> > available for a packet descriptor. The ring buffer's write_index
>
> The first sentence wording is a bit scrambled. I think you mean the
> ring buffer doesn't contain enough readable bytes to constitute a
> packet descriptor.
Indeed, replaced with your working.
> > is in memory which is shared with the Hyper-V host, its value is
> > thus subject to being changed at any time.
>
> This second sentence is true, but I'm not making the connection
> with the code change below. Evidently, there is some previous
> check made to ensure that enough bytes are available to be
> received when hvs_stream_dequeue() is called, so we assumed that
> NULL could never be returned? I looked but didn't find such a check,
> so maybe I didn't look carefully enough. But now we are assuming
> that Hyper-V might have invalidated that previous check by
> subsequently changing the write_index in a bogus way? So now, NULL
> could be returned when previously we assumed it couldn't.
I think you're looking for hvs_stream_has_data(). (Previous checks
apart, hvs_stream_dequeue() will "dereference" the pointer so...)
Thanks,
Andrea
From: Andrea Parri <[email protected]> Sent: Thursday, April 14, 2022 11:42 PM
>
> On Fri, Apr 15, 2022 at 03:33:23AM +0000, Michael Kelley (LINUX) wrote:
> > From: Andrea Parri (Microsoft) <[email protected]> Sent: Wednesday, April 13,
> 2022 1:48 PM
> > >
> > > The function returns NULL if the ring buffer has no enough space
> > > available for a packet descriptor. The ring buffer's write_index
> >
> > The first sentence wording is a bit scrambled. I think you mean the
> > ring buffer doesn't contain enough readable bytes to constitute a
> > packet descriptor.
>
> Indeed, replaced with your working.
>
>
> > > is in memory which is shared with the Hyper-V host, its value is
> > > thus subject to being changed at any time.
> >
> > This second sentence is true, but I'm not making the connection
> > with the code change below. Evidently, there is some previous
> > check made to ensure that enough bytes are available to be
> > received when hvs_stream_dequeue() is called, so we assumed that
> > NULL could never be returned? I looked but didn't find such a check,
> > so maybe I didn't look carefully enough. But now we are assuming
> > that Hyper-V might have invalidated that previous check by
> > subsequently changing the write_index in a bogus way? So now, NULL
> > could be returned when previously we assumed it couldn't.
>
> I think you're looking for hvs_stream_has_data(). (Previous checks
> apart, hvs_stream_dequeue() will "dereference" the pointer so...)
Agreed. I didn't say this explicitly, but I was wondering about the risk
in the current code (without these hardening patches) of getting a
NULL pointer from hv_pkt_iter_first_raw(), and then dereferencing it.
Michael
On Fri, Apr 15, 2022 at 02:27:37PM +0000, Michael Kelley (LINUX) wrote:
> From: Andrea Parri <[email protected]> Sent: Thursday, April 14, 2022 11:42 PM
> >
> > On Fri, Apr 15, 2022 at 03:33:23AM +0000, Michael Kelley (LINUX) wrote:
> > > From: Andrea Parri (Microsoft) <[email protected]> Sent: Wednesday, April 13,
> > 2022 1:48 PM
> > > >
> > > > The function returns NULL if the ring buffer has no enough space
> > > > available for a packet descriptor. The ring buffer's write_index
> > >
> > > The first sentence wording is a bit scrambled. I think you mean the
> > > ring buffer doesn't contain enough readable bytes to constitute a
> > > packet descriptor.
> >
> > Indeed, replaced with your working.
> >
> >
> > > > is in memory which is shared with the Hyper-V host, its value is
> > > > thus subject to being changed at any time.
> > >
> > > This second sentence is true, but I'm not making the connection
> > > with the code change below. Evidently, there is some previous
> > > check made to ensure that enough bytes are available to be
> > > received when hvs_stream_dequeue() is called, so we assumed that
> > > NULL could never be returned? I looked but didn't find such a check,
> > > so maybe I didn't look carefully enough. But now we are assuming
> > > that Hyper-V might have invalidated that previous check by
> > > subsequently changing the write_index in a bogus way? So now, NULL
> > > could be returned when previously we assumed it couldn't.
> >
> > I think you're looking for hvs_stream_has_data(). (Previous checks
> > apart, hvs_stream_dequeue() will "dereference" the pointer so...)
>
> Agreed. I didn't say this explicitly, but I was wondering about the risk
> in the current code (without these hardening patches) of getting a
> NULL pointer from hv_pkt_iter_first_raw(), and then dereferencing it.
Got it. Updated the changelog to:
"The ring buffer's write_index is in memory which is shared with the
Hyper-V host, an erroneous or malicious host could thus change its
value and overturn the result of hvs_stream_has_data()."
Hopefully this can clarify the issue (without introducing other typos).
Thanks,
Andrea
From: Andrea Parri (Microsoft) <[email protected]> Sent: Wednesday, April 13, 2022 1:48 PM
>
> The function returns NULL if the ring buffer has no enough space
> available for a packet descriptor. The ring buffer's write_index
The first sentence wording is a bit scrambled. I think you mean the
ring buffer doesn't contain enough readable bytes to constitute a
packet descriptor.
> is in memory which is shared with the Hyper-V host, its value is
> thus subject to being changed at any time.
This second sentence is true, but I'm not making the connection
with the code change below. Evidently, there is some previous
check made to ensure that enough bytes are available to be
received when hvs_stream_dequeue() is called, so we assumed that
NULL could never be returned? I looked but didn't find such a check,
so maybe I didn't look carefully enough. But now we are assuming
that Hyper-V might have invalidated that previous check by
subsequently changing the write_index in a bogus way? So now, NULL
could be returned when previously we assumed it couldn't.
Michael
>
> Signed-off-by: Andrea Parri (Microsoft) <[email protected]>
> ---
> net/vmw_vsock/hyperv_transport.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
> index e111e13b66604..943352530936e 100644
> --- a/net/vmw_vsock/hyperv_transport.c
> +++ b/net/vmw_vsock/hyperv_transport.c
> @@ -603,6 +603,8 @@ static ssize_t hvs_stream_dequeue(struct vsock_sock *vsk,
> struct msghdr *msg,
>
> if (need_refill) {
> hvs->recv_desc = hv_pkt_iter_first_raw(hvs->chan);
> + if (!hvs->recv_desc)
> + return -ENOBUFS;
> ret = hvs_update_recv_data(hvs);
> if (ret)
> return ret;
> --
> 2.25.1