diff mbox series

team: Fix use-after-free when an option instance allocation fails

Message ID 20231206123719.1963153-1-revest@chromium.org
State New
Headers
Series team: Fix use-after-free when an option instance allocation fails |

Commit Message

Florent Revest Dec. 6, 2023, 12:37 p.m. UTC 
  In __team_options_register, team_options are allocated and appended to
the team's option_list.
If one option instance allocation fails, the "inst_rollback" cleanup
path frees the previously allocated options but doesn't remove them from
the team's option_list.
This leaves dangling pointers that can be dereferenced later by other
parts of the team driver that iterate over options.

This patch fixes the cleanup path to remove the dangling pointers from
the list.

As far as I can tell, this uaf doesn't have much security implications
since it would be fairly hard to exploit (an attacker would need to make
the allocation of that specific small object fail) but it's still nice
to fix.

Fixes: 80f7c6683fe0 ("team: add support for per-port options")
Signed-off-by: Florent Revest <revest@chromium.org>
---
 drivers/net/team/team.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Jiri Pirko Dec. 6, 2023, 1:21 p.m. UTC | #1 
Wed, Dec 06, 2023 at 01:37:18PM CET, revest@chromium.org wrote:
>In __team_options_register, team_options are allocated and appended to
>the team's option_list.
>If one option instance allocation fails, the "inst_rollback" cleanup
>path frees the previously allocated options but doesn't remove them from
>the team's option_list.
>This leaves dangling pointers that can be dereferenced later by other
>parts of the team driver that iterate over options.
>
>This patch fixes the cleanup path to remove the dangling pointers from
>the list.
>
>As far as I can tell, this uaf doesn't have much security implications
>since it would be fairly hard to exploit (an attacker would need to make
>the allocation of that specific small object fail) but it's still nice
>to fix.
>
>Fixes: 80f7c6683fe0 ("team: add support for per-port options")
>Signed-off-by: Florent Revest <revest@chromium.org>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

Thanks!
Hangbin Liu Dec. 6, 2023, 3:05 p.m. UTC | #2 
On Wed, Dec 06, 2023 at 01:37:18PM +0100, Florent Revest wrote:
> In __team_options_register, team_options are allocated and appended to
> the team's option_list.
> If one option instance allocation fails, the "inst_rollback" cleanup
> path frees the previously allocated options but doesn't remove them from
> the team's option_list.
> This leaves dangling pointers that can be dereferenced later by other
> parts of the team driver that iterate over options.
> 
> This patch fixes the cleanup path to remove the dangling pointers from
> the list.
> 
> As far as I can tell, this uaf doesn't have much security implications
> since it would be fairly hard to exploit (an attacker would need to make
> the allocation of that specific small object fail) but it's still nice
> to fix.
> 
> Fixes: 80f7c6683fe0 ("team: add support for per-port options")
> Signed-off-by: Florent Revest <revest@chromium.org>
> ---
>  drivers/net/team/team.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
> index 508d9a392ab18..f575f225d4178 100644
> --- a/drivers/net/team/team.c
> +++ b/drivers/net/team/team.c
> @@ -281,8 +281,10 @@ static int __team_options_register(struct team *team,
>  	return 0;
>  
>  inst_rollback:
> -	for (i--; i >= 0; i--)
> +	for (i--; i >= 0; i--) {
>  		__team_option_inst_del_option(team, dst_opts[i]);
> +		list_del(&dst_opts[i]->list);
> +	}
>  
>  	i = option_count;
>  alloc_rollback:
> -- 
> 2.43.0.rc2.451.g8631bc7472-goog
> 

Reviewed-by: Hangbin Liu <liuhangbin@gmail.com>
Florent Revest Dec. 6, 2023, 4:31 p.m. UTC | #3 
On Wed, Dec 6, 2023 at 4:05 PM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> On Wed, Dec 06, 2023 at 01:37:18PM +0100, Florent Revest wrote:
> > In __team_options_register, team_options are allocated and appended to
> > the team's option_list.
> > If one option instance allocation fails, the "inst_rollback" cleanup
> > path frees the previously allocated options but doesn't remove them from
> > the team's option_list.
> > This leaves dangling pointers that can be dereferenced later by other
> > parts of the team driver that iterate over options.
> >
> > This patch fixes the cleanup path to remove the dangling pointers from
> > the list.
> >
> > As far as I can tell, this uaf doesn't have much security implications
> > since it would be fairly hard to exploit (an attacker would need to make
> > the allocation of that specific small object fail) but it's still nice
> > to fix.
> >
> > Fixes: 80f7c6683fe0 ("team: add support for per-port options")
> > Signed-off-by: Florent Revest <revest@chromium.org>
>
> Reviewed-by: Hangbin Liu <liuhangbin@gmail.com>

Thank you for the quick reviews Hangbin & Jiri, I appreciate! :)

I just realized I forgot to CC stable (like I always do... :) maybe I
should tattoo it on my arm) Let me know if you'd like a v2 adding:

Cc: stable@vger.kernel.org
Hangbin Liu Dec. 7, 2023, 1:02 a.m. UTC | #4 
On Wed, Dec 06, 2023 at 05:31:58PM +0100, Florent Revest wrote:
> Thank you for the quick reviews Hangbin & Jiri, I appreciate! :)
> 
> I just realized I forgot to CC stable (like I always do... :) maybe I
> should tattoo it on my arm) Let me know if you'd like a v2 adding:
> 
> Cc: stable@vger.kernel.org

I think Greg will take care of it. No need to send v2 when there is
nothing to change.

Thanks
Hangbin
patchwork-bot+netdevbpf@kernel.org Dec. 8, 2023, 6:50 p.m. UTC | #5 
Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed,  6 Dec 2023 13:37:18 +0100 you wrote:
> In __team_options_register, team_options are allocated and appended to
> the team's option_list.
> If one option instance allocation fails, the "inst_rollback" cleanup
> path frees the previously allocated options but doesn't remove them from
> the team's option_list.
> This leaves dangling pointers that can be dereferenced later by other
> parts of the team driver that iterate over options.
> 
> [...]

Here is the summary with links:
  - team: Fix use-after-free when an option instance allocation fails
    https://git.kernel.org/netdev/net/c/c12296bbecc4

You are awesome, thank you!
diff mbox series

Patch

diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index 508d9a392ab18..f575f225d4178 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -281,8 +281,10 @@  static int __team_options_register(struct team *team,
 	return 0;
 
 inst_rollback:
-	for (i--; i >= 0; i--)
+	for (i--; i >= 0; i--) {
 		__team_option_inst_del_option(team, dst_opts[i]);
+		list_del(&dst_opts[i]->list);
+	}
 
 	i = option_count;
 alloc_rollback: