Re: [PATCH] Drivers: hv: vmbus: Use after free in __vmbus_open()
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 _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 > > I had this 'queued' in my list, > > Reviewed-by: Andrea Parri Applied. Thanks.
Re: [PATCH] Drivers: hv: vmbus: Use after free in __vmbus_open()
On Tue, Apr 13, 2021 at 01:50:04PM +0300, Dan Carpenter wrote: > The "open_info" variable is added to the _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 I had this 'queued' in my list, Reviewed-by: Andrea Parri 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 >
Re: [PATCH] Drivers: hv: vmbus: Use after free in __vmbus_open()
On Tue, Apr 13, 2021 at 01:50:04PM +0300, Dan Carpenter wrote: > The "open_info" variable is added to the _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 > --- > 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 >
[PATCH] Drivers: hv: vmbus: Use after free in __vmbus_open()
The "open_info" variable is added to the _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 --- >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