* [PATCH AUTOSEL 5.11 50/67] Drivers: hv: vmbus: Initialize memory to be sent to the host [not found] <20210224125026.481804-1-sashal@kernel.org> @ 2021-02-24 12:50 ` Sasha Levin 2021-02-24 13:16 ` Andrea Parri 2021-02-24 12:50 ` [PATCH AUTOSEL 5.11 51/67] Drivers: hv: vmbus: Resolve race condition in vmbus_onoffer_rescind() Sasha Levin 1 sibling, 1 reply; 6+ messages in thread From: Sasha Levin @ 2021-02-24 12:50 UTC (permalink / raw) To: linux-kernel, stable Cc: Andrea Parri (Microsoft), Juan Vazquez, Michael Kelley, Wei Liu, Sasha Levin, linux-hyperv From: "Andrea Parri (Microsoft)" <parri.andrea@gmail.com> [ Upstream commit e99c4afbee07e9323e9191a20b24d74dbf815bdf ] __vmbus_open() and vmbus_teardown_gpadl() do not inizialite the memory for the vmbus_channel_open_channel and the vmbus_channel_gpadl_teardown objects they allocate respectively. These objects contain padding bytes and fields that are left uninitialized and that are later sent to the host, potentially leaking guest data. Zero initialize such fields to avoid leaking sensitive information to the host. Reported-by: Juan Vazquez <juvazq@microsoft.com> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Reviewed-by: Michael Kelley <mikelley@microsoft.com> Link: https://lore.kernel.org/r/20201209070827.29335-2-parri.andrea@gmail.com Signed-off-by: Wei Liu <wei.liu@kernel.org> Signed-off-by: Sasha Levin <sashal@kernel.org> --- drivers/hv/channel.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c index 6fb0c76bfbf81..0bd202de79600 100644 --- a/drivers/hv/channel.c +++ b/drivers/hv/channel.c @@ -618,7 +618,7 @@ static int __vmbus_open(struct vmbus_channel *newchannel, goto error_clean_ring; /* Create and init the channel open message */ - open_info = kmalloc(sizeof(*open_info) + + open_info = kzalloc(sizeof(*open_info) + sizeof(struct vmbus_channel_open_channel), GFP_KERNEL); if (!open_info) { @@ -745,7 +745,7 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32 gpadl_handle) unsigned long flags; int ret; - info = kmalloc(sizeof(*info) + + info = kzalloc(sizeof(*info) + sizeof(struct vmbus_channel_gpadl_teardown), GFP_KERNEL); if (!info) return -ENOMEM; -- 2.27.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH AUTOSEL 5.11 50/67] Drivers: hv: vmbus: Initialize memory to be sent to the host 2021-02-24 12:50 ` [PATCH AUTOSEL 5.11 50/67] Drivers: hv: vmbus: Initialize memory to be sent to the host Sasha Levin @ 2021-02-24 13:16 ` Andrea Parri 2021-02-24 13:30 ` Andrea Parri 0 siblings, 1 reply; 6+ messages in thread From: Andrea Parri @ 2021-02-24 13:16 UTC (permalink / raw) To: Sasha Levin Cc: linux-kernel, stable, Juan Vazquez, Michael Kelley, Wei Liu, linux-hyperv On Wed, Feb 24, 2021 at 07:50:08AM -0500, Sasha Levin wrote: > From: "Andrea Parri (Microsoft)" <parri.andrea@gmail.com> > > [ Upstream commit e99c4afbee07e9323e9191a20b24d74dbf815bdf ] > > __vmbus_open() and vmbus_teardown_gpadl() do not inizialite the memory > for the vmbus_channel_open_channel and the vmbus_channel_gpadl_teardown > objects they allocate respectively. These objects contain padding bytes > and fields that are left uninitialized and that are later sent to the > host, potentially leaking guest data. Zero initialize such fields to > avoid leaking sensitive information to the host. > > Reported-by: Juan Vazquez <juvazq@microsoft.com> > Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com> > Reviewed-by: Michael Kelley <mikelley@microsoft.com> > Link: https://lore.kernel.org/r/20201209070827.29335-2-parri.andrea@gmail.com > Signed-off-by: Wei Liu <wei.liu@kernel.org> > Signed-off-by: Sasha Levin <sashal@kernel.org> Sasha - This patch is one of a group of patches where a Linux guest running on Hyper-V will start assuming that hypervisor behavior might be malicious, and guards against such behavior. Because this is a new assumption, these patches are more properly treated as new functionality rather than as bug fixes. So I would propose that we *not* bring such patches back to stable branches. Thanks, Andrea > --- > drivers/hv/channel.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c > index 6fb0c76bfbf81..0bd202de79600 100644 > --- a/drivers/hv/channel.c > +++ b/drivers/hv/channel.c > @@ -618,7 +618,7 @@ static int __vmbus_open(struct vmbus_channel *newchannel, > goto error_clean_ring; > > /* Create and init the channel open message */ > - open_info = kmalloc(sizeof(*open_info) + > + open_info = kzalloc(sizeof(*open_info) + > sizeof(struct vmbus_channel_open_channel), > GFP_KERNEL); > if (!open_info) { > @@ -745,7 +745,7 @@ int vmbus_teardown_gpadl(struct vmbus_channel *channel, u32 gpadl_handle) > unsigned long flags; > int ret; > > - info = kmalloc(sizeof(*info) + > + info = kzalloc(sizeof(*info) + > sizeof(struct vmbus_channel_gpadl_teardown), GFP_KERNEL); > if (!info) > return -ENOMEM; > -- > 2.27.0 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH AUTOSEL 5.11 50/67] Drivers: hv: vmbus: Initialize memory to be sent to the host 2021-02-24 13:16 ` Andrea Parri @ 2021-02-24 13:30 ` Andrea Parri 2021-02-26 17:05 ` Sasha Levin 0 siblings, 1 reply; 6+ messages in thread From: Andrea Parri @ 2021-02-24 13:30 UTC (permalink / raw) To: Sasha Levin Cc: linux-kernel, stable, Juan Vazquez, Michael Kelley, Wei Liu, linux-hyperv On Wed, Feb 24, 2021 at 02:16:00PM +0100, Andrea Parri wrote: > On Wed, Feb 24, 2021 at 07:50:08AM -0500, Sasha Levin wrote: > > From: "Andrea Parri (Microsoft)" <parri.andrea@gmail.com> > > > > [ Upstream commit e99c4afbee07e9323e9191a20b24d74dbf815bdf ] > > > > __vmbus_open() and vmbus_teardown_gpadl() do not inizialite the memory > > for the vmbus_channel_open_channel and the vmbus_channel_gpadl_teardown > > objects they allocate respectively. These objects contain padding bytes > > and fields that are left uninitialized and that are later sent to the > > host, potentially leaking guest data. Zero initialize such fields to > > avoid leaking sensitive information to the host. > > > > Reported-by: Juan Vazquez <juvazq@microsoft.com> > > Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com> > > Reviewed-by: Michael Kelley <mikelley@microsoft.com> > > Link: https://lore.kernel.org/r/20201209070827.29335-2-parri.andrea@gmail.com > > Signed-off-by: Wei Liu <wei.liu@kernel.org> > > Signed-off-by: Sasha Levin <sashal@kernel.org> > > Sasha - This patch is one of a group of patches where a Linux guest running on > Hyper-V will start assuming that hypervisor behavior might be malicious, and > guards against such behavior. Because this is a new assumption, these patches > are more properly treated as new functionality rather than as bug fixes. So I > would propose that we *not* bring such patches back to stable branches. For future/similar cases: I'm wondering, is there some way to annotate a patch with "please do not bring it back"? Thanks, Andrea ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH AUTOSEL 5.11 50/67] Drivers: hv: vmbus: Initialize memory to be sent to the host 2021-02-24 13:30 ` Andrea Parri @ 2021-02-26 17:05 ` Sasha Levin 0 siblings, 0 replies; 6+ messages in thread From: Sasha Levin @ 2021-02-26 17:05 UTC (permalink / raw) To: Andrea Parri Cc: linux-kernel, stable, Juan Vazquez, Michael Kelley, Wei Liu, linux-hyperv On Wed, Feb 24, 2021 at 02:30:52PM +0100, Andrea Parri wrote: >On Wed, Feb 24, 2021 at 02:16:00PM +0100, Andrea Parri wrote: >> On Wed, Feb 24, 2021 at 07:50:08AM -0500, Sasha Levin wrote: >> > From: "Andrea Parri (Microsoft)" <parri.andrea@gmail.com> >> > >> > [ Upstream commit e99c4afbee07e9323e9191a20b24d74dbf815bdf ] >> > >> > __vmbus_open() and vmbus_teardown_gpadl() do not inizialite the memory >> > for the vmbus_channel_open_channel and the vmbus_channel_gpadl_teardown >> > objects they allocate respectively. These objects contain padding bytes >> > and fields that are left uninitialized and that are later sent to the >> > host, potentially leaking guest data. Zero initialize such fields to >> > avoid leaking sensitive information to the host. >> > >> > Reported-by: Juan Vazquez <juvazq@microsoft.com> >> > Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com> >> > Reviewed-by: Michael Kelley <mikelley@microsoft.com> >> > Link: https://lore.kernel.org/r/20201209070827.29335-2-parri.andrea@gmail.com >> > Signed-off-by: Wei Liu <wei.liu@kernel.org> >> > Signed-off-by: Sasha Levin <sashal@kernel.org> >> >> Sasha - This patch is one of a group of patches where a Linux guest running on >> Hyper-V will start assuming that hypervisor behavior might be malicious, and >> guards against such behavior. Because this is a new assumption, these patches >> are more properly treated as new functionality rather than as bug fixes. So I >> would propose that we *not* bring such patches back to stable branches. > >For future/similar cases: I'm wondering, is there some way to annotate a patch >with "please do not bring it back"? There's nothing explicit for the AUTOSEL stuff. A note in the changelog could work. -- Thanks, Sasha ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH AUTOSEL 5.11 51/67] Drivers: hv: vmbus: Resolve race condition in vmbus_onoffer_rescind() [not found] <20210224125026.481804-1-sashal@kernel.org> 2021-02-24 12:50 ` [PATCH AUTOSEL 5.11 50/67] Drivers: hv: vmbus: Initialize memory to be sent to the host Sasha Levin @ 2021-02-24 12:50 ` Sasha Levin 2021-02-24 13:17 ` Andrea Parri 1 sibling, 1 reply; 6+ messages in thread From: Sasha Levin @ 2021-02-24 12:50 UTC (permalink / raw) To: linux-kernel, stable Cc: Andrea Parri (Microsoft), Juan Vazquez, Michael Kelley, Wei Liu, Sasha Levin, linux-hyperv From: "Andrea Parri (Microsoft)" <parri.andrea@gmail.com> [ Upstream commit e4d221b42354b2e2ddb9187a806afb651eee2cda ] An erroneous or malicious host could send multiple rescind messages for a same channel. In vmbus_onoffer_rescind(), the guest maps the channel ID to obtain a pointer to the channel object and it eventually releases such object and associated data. The host could time rescind messages and lead to an use-after-free. Add a new flag to the channel structure to make sure that only one instance of vmbus_onoffer_rescind() can get the reference to the channel object. Reported-by: Juan Vazquez <juvazq@microsoft.com> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Reviewed-by: Michael Kelley <mikelley@microsoft.com> Link: https://lore.kernel.org/r/20201209070827.29335-6-parri.andrea@gmail.com Signed-off-by: Wei Liu <wei.liu@kernel.org> Signed-off-by: Sasha Levin <sashal@kernel.org> --- drivers/hv/channel_mgmt.c | 12 ++++++++++++ include/linux/hyperv.h | 1 + 2 files changed, 13 insertions(+) diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c index 1d44bb635bb84..a9f58840f85dc 100644 --- a/drivers/hv/channel_mgmt.c +++ b/drivers/hv/channel_mgmt.c @@ -1049,6 +1049,18 @@ static void vmbus_onoffer_rescind(struct vmbus_channel_message_header *hdr) mutex_lock(&vmbus_connection.channel_mutex); channel = relid2channel(rescind->child_relid); + if (channel != NULL) { + /* + * Guarantee that no other instance of vmbus_onoffer_rescind() + * has got a reference to the channel object. Synchronize on + * &vmbus_connection.channel_mutex. + */ + if (channel->rescind_ref) { + mutex_unlock(&vmbus_connection.channel_mutex); + return; + } + channel->rescind_ref = true; + } mutex_unlock(&vmbus_connection.channel_mutex); if (channel == NULL) { diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h index 5ddb479c4d4cb..ef3573e99d989 100644 --- a/include/linux/hyperv.h +++ b/include/linux/hyperv.h @@ -803,6 +803,7 @@ struct vmbus_channel { u8 monitor_bit; bool rescind; /* got rescind msg */ + bool rescind_ref; /* got rescind msg, got channel reference */ struct completion rescind_event; u32 ringbuffer_gpadlhandle; -- 2.27.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH AUTOSEL 5.11 51/67] Drivers: hv: vmbus: Resolve race condition in vmbus_onoffer_rescind() 2021-02-24 12:50 ` [PATCH AUTOSEL 5.11 51/67] Drivers: hv: vmbus: Resolve race condition in vmbus_onoffer_rescind() Sasha Levin @ 2021-02-24 13:17 ` Andrea Parri 0 siblings, 0 replies; 6+ messages in thread From: Andrea Parri @ 2021-02-24 13:17 UTC (permalink / raw) To: Sasha Levin Cc: linux-kernel, stable, Juan Vazquez, Michael Kelley, Wei Liu, linux-hyperv On Wed, Feb 24, 2021 at 07:50:09AM -0500, Sasha Levin wrote: > From: "Andrea Parri (Microsoft)" <parri.andrea@gmail.com> > > [ Upstream commit e4d221b42354b2e2ddb9187a806afb651eee2cda ] > > An erroneous or malicious host could send multiple rescind messages for > a same channel. In vmbus_onoffer_rescind(), the guest maps the channel > ID to obtain a pointer to the channel object and it eventually releases > such object and associated data. The host could time rescind messages > and lead to an use-after-free. Add a new flag to the channel structure > to make sure that only one instance of vmbus_onoffer_rescind() can get > the reference to the channel object. > > Reported-by: Juan Vazquez <juvazq@microsoft.com> > Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com> > Reviewed-by: Michael Kelley <mikelley@microsoft.com> > Link: https://lore.kernel.org/r/20201209070827.29335-6-parri.andrea@gmail.com > Signed-off-by: Wei Liu <wei.liu@kernel.org> > Signed-off-by: Sasha Levin <sashal@kernel.org> Sasha - This patch is one of a group of patches where a Linux guest running on Hyper-V will start assuming that hypervisor behavior might be malicious, and guards against such behavior. Because this is a new assumption, these patches are more properly treated as new functionality rather than as bug fixes. So I would propose that we *not* bring such patches back to stable branches. Thanks, Andrea > --- > drivers/hv/channel_mgmt.c | 12 ++++++++++++ > include/linux/hyperv.h | 1 + > 2 files changed, 13 insertions(+) > > diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c > index 1d44bb635bb84..a9f58840f85dc 100644 > --- a/drivers/hv/channel_mgmt.c > +++ b/drivers/hv/channel_mgmt.c > @@ -1049,6 +1049,18 @@ static void vmbus_onoffer_rescind(struct vmbus_channel_message_header *hdr) > > mutex_lock(&vmbus_connection.channel_mutex); > channel = relid2channel(rescind->child_relid); > + if (channel != NULL) { > + /* > + * Guarantee that no other instance of vmbus_onoffer_rescind() > + * has got a reference to the channel object. Synchronize on > + * &vmbus_connection.channel_mutex. > + */ > + if (channel->rescind_ref) { > + mutex_unlock(&vmbus_connection.channel_mutex); > + return; > + } > + channel->rescind_ref = true; > + } > mutex_unlock(&vmbus_connection.channel_mutex); > > if (channel == NULL) { > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h > index 5ddb479c4d4cb..ef3573e99d989 100644 > --- a/include/linux/hyperv.h > +++ b/include/linux/hyperv.h > @@ -803,6 +803,7 @@ struct vmbus_channel { > u8 monitor_bit; > > bool rescind; /* got rescind msg */ > + bool rescind_ref; /* got rescind msg, got channel reference */ > struct completion rescind_event; > > u32 ringbuffer_gpadlhandle; > -- > 2.27.0 > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-02-26 17:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20210224125026.481804-1-sashal@kernel.org>
2021-02-24 12:50 ` [PATCH AUTOSEL 5.11 50/67] Drivers: hv: vmbus: Initialize memory to be sent to the host Sasha Levin
2021-02-24 13:16 ` Andrea Parri
2021-02-24 13:30 ` Andrea Parri
2021-02-26 17:05 ` Sasha Levin
2021-02-24 12:50 ` [PATCH AUTOSEL 5.11 51/67] Drivers: hv: vmbus: Resolve race condition in vmbus_onoffer_rescind() Sasha Levin
2021-02-24 13:17 ` Andrea Parri
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).