Linux-HyperV Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] Drivers: hv: vmbus: Use after free in __vmbus_open()
@ 2021-04-13 10:50 Dan Carpenter
  2021-04-13 11:46 ` Wei Liu
  2021-04-13 15:42 ` Andrea Parri
  0 siblings, 2 replies; 4+ messages in thread
From: Dan Carpenter @ 2021-04-13 10:50 UTC (permalink / raw)
  To: K. Y. Srinivasan
  Cc: Haiyang Zhang, Stephen Hemminger, Wei Liu, Greg Kroah-Hartman,
	Dexuan Cui, linux-hyperv, linux-kernel, kernel-janitors

The "open_info" variable is added to the &vmbus_connection.chn_msg_list,
but the error handling frees "open_info" without removing it from the
list.  This will result in a use after free.  First remove it from the
list, and then free it.

Fixes: 6f3d791f3006 ("Drivers: hv: vmbus: Fix rescind handling issues")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
From static analysis.  Untested etc.  There is almost certainly a good
reason to add it to the list before checking "newchannel->rescind" but I
don't know the code well enough to know what the reason is.

 drivers/hv/channel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
index db30be8f9cce..1c5a418c1962 100644
--- a/drivers/hv/channel.c
+++ b/drivers/hv/channel.c
@@ -653,7 +653,7 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
 
 	if (newchannel->rescind) {
 		err = -ENODEV;
-		goto error_free_info;
+		goto error_clean_msglist;
 	}
 
 	err = vmbus_post_msg(open_msg,
-- 
2.30.2


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] Drivers: hv: vmbus: Use after free in __vmbus_open()
  2021-04-13 10:50 [PATCH] Drivers: hv: vmbus: Use after free in __vmbus_open() Dan Carpenter
@ 2021-04-13 11:46 ` Wei Liu
  2021-04-13 15:42 ` Andrea Parri
  1 sibling, 0 replies; 4+ messages in thread
From: Wei Liu @ 2021-04-13 11:46 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Greg Kroah-Hartman, Dexuan Cui, linux-hyperv, linux-kernel,
	kernel-janitors, Michael Kelley

On Tue, Apr 13, 2021 at 01:50:04PM +0300, Dan Carpenter wrote:
> The "open_info" variable is added to the &vmbus_connection.chn_msg_list,
> but the error handling frees "open_info" without removing it from the
> list.  This will result in a use after free.  First remove it from the
> list, and then free it.
> 
> Fixes: 6f3d791f3006 ("Drivers: hv: vmbus: Fix rescind handling issues")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> From static analysis.  Untested etc.  There is almost certainly a good
> reason to add it to the list before checking "newchannel->rescind" but I
> don't know the code well enough to know what the reason is.
> 

AIUI the channel management code requires the message be queued before
posting the message to backend, because processing response is done in
another thread, and might happen before this message is added to the
list if the order is reversed.

>  drivers/hv/channel.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index db30be8f9cce..1c5a418c1962 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -653,7 +653,7 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
>  
>  	if (newchannel->rescind) {
>  		err = -ENODEV;
> -		goto error_free_info;
> +		goto error_clean_msglist;

Looking at similar functions in the same file I think there is indeed an
UAF problem in the original code.

I have not studied this piece of code extensively so I will wait for
others to chime in.

Wei.

>  	}
>  
>  	err = vmbus_post_msg(open_msg,
> -- 
> 2.30.2
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] Drivers: hv: vmbus: Use after free in __vmbus_open()
  2021-04-13 10:50 [PATCH] Drivers: hv: vmbus: Use after free in __vmbus_open() Dan Carpenter
  2021-04-13 11:46 ` Wei Liu
@ 2021-04-13 15:42 ` Andrea Parri
  2021-04-16 10:37   ` Wei Liu
  1 sibling, 1 reply; 4+ messages in thread
From: Andrea Parri @ 2021-04-13 15:42 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: K. Y. Srinivasan, Haiyang Zhang, Stephen Hemminger, Wei Liu,
	Greg Kroah-Hartman, Dexuan Cui, linux-hyperv, linux-kernel,
	kernel-janitors

On Tue, Apr 13, 2021 at 01:50:04PM +0300, Dan Carpenter wrote:
> The "open_info" variable is added to the &vmbus_connection.chn_msg_list,
> but the error handling frees "open_info" without removing it from the
> list.  This will result in a use after free.  First remove it from the
> list, and then free it.
> 
> Fixes: 6f3d791f3006 ("Drivers: hv: vmbus: Fix rescind handling issues")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

I had this 'queued' in my list,

Reviewed-by: Andrea Parri <parri.andrea@gmail.com>

  Andrea


> ---
> From static analysis.  Untested etc.  There is almost certainly a good
> reason to add it to the list before checking "newchannel->rescind" but I
> don't know the code well enough to know what the reason is.
> 
>  drivers/hv/channel.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index db30be8f9cce..1c5a418c1962 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -653,7 +653,7 @@ static int __vmbus_open(struct vmbus_channel *newchannel,
>  
>  	if (newchannel->rescind) {
>  		err = -ENODEV;
> -		goto error_free_info;
> +		goto error_clean_msglist;
>  	}
>  
>  	err = vmbus_post_msg(open_msg,
> -- 
> 2.30.2
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] Drivers: hv: vmbus: Use after free in __vmbus_open()
  2021-04-13 15:42 ` Andrea Parri
@ 2021-04-16 10:37   ` Wei Liu
  0 siblings, 0 replies; 4+ messages in thread
From: Wei Liu @ 2021-04-16 10:37 UTC (permalink / raw)
  To: Andrea Parri
  Cc: Dan Carpenter, K. Y. Srinivasan, Haiyang Zhang,
	Stephen Hemminger, Wei Liu, Greg Kroah-Hartman, Dexuan Cui,
	linux-hyperv, linux-kernel, kernel-janitors

On Tue, Apr 13, 2021 at 05:42:21PM +0200, Andrea Parri wrote:
> On Tue, Apr 13, 2021 at 01:50:04PM +0300, Dan Carpenter wrote:
> > The "open_info" variable is added to the &vmbus_connection.chn_msg_list,
> > but the error handling frees "open_info" without removing it from the
> > list.  This will result in a use after free.  First remove it from the
> > list, and then free it.
> > 
> > Fixes: 6f3d791f3006 ("Drivers: hv: vmbus: Fix rescind handling issues")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> I had this 'queued' in my list,
> 
> Reviewed-by: Andrea Parri <parri.andrea@gmail.com>

Applied. Thanks.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-13 10:50 [PATCH] Drivers: hv: vmbus: Use after free in __vmbus_open() Dan Carpenter
2021-04-13 11:46 ` Wei Liu
2021-04-13 15:42 ` Andrea Parri
2021-04-16 10:37   ` Wei Liu

Linux-HyperV Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hyperv/0 linux-hyperv/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hyperv linux-hyperv/ https://lore.kernel.org/linux-hyperv \
		linux-hyperv@vger.kernel.org
	public-inbox-index linux-hyperv

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hyperv


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git