| Message ID | 20220413204742.5539-2-parri.andrea@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | hv_sock: Hardening changes | expand |
From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> 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) <parri.andrea@gmail.com> > --- > 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) <parri.andrea@gmail.com> 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 <parri.andrea@gmail.com> 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) <parri.andrea@gmail.com> 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 <parri.andrea@gmail.com> 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) <parri.andrea@gmail.com> 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
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;
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) <parri.andrea@gmail.com> --- net/vmw_vsock/hyperv_transport.c | 2 ++ 1 file changed, 2 insertions(+)