Drivers: hv: vmbus: Use after free in __vmbus_open()
PATCH 1/1 Drivers: hv: vmbus: Use after free in __vmbus_open()
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.
---
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(-)
drivers/hv/channel.c +1 -1
@@ -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,
Signature · 1 line
2.30.2
Wei Liu
maintainerQuoted 5 lines · Dan Carpenter
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.
Quoted 7 lines
Fixes 6f3d791f3006 ("Drivers: hv: vmbus: Fix rescind handling issues")Signed-off-by Dan Carpenter <[email protected]>---
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 static int __vmbus_open(struct vmbus_channel *newchannel, · line 656
Quoted 13 lines
drivers/hv/channel.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/hv/channel.c +1
-1@@ -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.
Quoted 6 lines
drivers/hv/channel.c +0
-0} err = vmbus_post_msg(open_msg,Signature · 1 line
2.30.2
Quoted 7 lines · Dan Carpenter
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 <[email protected]>
I had this 'queued' in my list,
Andrea
Quoted 24 lines
---
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(-)drivers/hv/channel.c +1
-1@@ -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,Signature · 1 line
2.30.2
Quoted 6 lines · Andrea Parri
Quoted 5 lines · Dan Carpenter
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.
Quoted 6 lines
Quoted 2 lines
Fixes 6f3d791f3006 ("Drivers: hv: vmbus: Fix rescind handling issues")Signed-off-by Dan Carpenter <[email protected]>I had this 'queued' in my list,
Reviewed-by Andrea Parri <[email protected]>
Applied. Thanks.