[PATCH net] net: genl: fix error path memory leak in policy dumping

[PATCH net] net: genl: fix error path memory leak in policy dumping

[Jakub Kicinski]

If construction of the array of policies fails when recording
non-first policy we need to unwind.

Reported-by: syzbot+dc54d9ba8153b216cae0@syzkaller.appspotmail.com
Fixes: 50a896cf2d6f ("genetlink: properly support per-op policy dumping")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 net/netlink/genetlink.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 1afca2a6c2ac..57010927e20a 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -1174,13 +1174,17 @@ static int ctrl_dumppolicy_start(struct netlink_callback *cb)
 							     op.policy,
 							     op.maxattr);
 			if (err)
-				return err;
+				goto err_free_state;
 		}
 	}
 
 	if (!ctx->state)
 		return -ENODATA;
 	return 0;
+
+err_free_state:
+	netlink_policy_dump_free(ctx->state);
+	return err;
 }
 
 static void *ctrl_dumppolicy_prep(struct sk_buff *skb,
-- 
2.37.2

Re: [PATCH net] net: genl: fix error path memory leak in policy dumping

[Rafael Soares]

On Mon, Aug 15, 2022 at 11:20:21AM -0700, Jakub Kicinski wrote:
> If construction of the array of policies fails when recording
> non-first policy we need to unwind.
> 
> Reported-by: syzbot+dc54d9ba8153b216cae0@syzkaller.appspotmail.com
> Fixes: 50a896cf2d6f ("genetlink: properly support per-op policy dumping")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  net/netlink/genetlink.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> index 1afca2a6c2ac..57010927e20a 100644
> --- a/net/netlink/genetlink.c
> +++ b/net/netlink/genetlink.c
> @@ -1174,13 +1174,17 @@ static int ctrl_dumppolicy_start(struct netlink_callback *cb)
>  							     op.policy,
>  							     op.maxattr);
>  			if (err)
> -				return err;
> +				goto err_free_state;

There's another call to netlink_policy_dump_add_policy() right above
this one. The patch I posted to syzkaller frees the memory inside
netlink_policy_dump_add_policy() and the result was OK.

>  		}
>  	}
>  
>  	if (!ctx->state)
>  		return -ENODATA;
>  	return 0;
> +
> +err_free_state:
> +	netlink_policy_dump_free(ctx->state);
> +	return err;
>  }
>  
>  static void *ctrl_dumppolicy_prep(struct sk_buff *skb,
> -- 
> 2.37.2
> 

Re: [PATCH net] net: genl: fix error path memory leak in policy dumping

[Jakub Kicinski]

On Mon, 15 Aug 2022 11:20:21 -0700 Jakub Kicinski wrote:
> If construction of the array of policies fails when recording
> non-first policy we need to unwind.
> 
> Reported-by: syzbot+dc54d9ba8153b216cae0@syzkaller.appspotmail.com
> Fixes: 50a896cf2d6f ("genetlink: properly support per-op policy dumping")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

syzbot says still pooped, indeed there's more leaks in
netlink_policy_dump_add_policy() itself. v2 coming...

Re: [PATCH net] net: genl: fix error path memory leak in policy dumping

[Jakub Kicinski]

On Mon, 15 Aug 2022 18:08:58 -0300 Rafael Soares wrote:
> On Mon, Aug 15, 2022 at 11:20:21AM -0700, Jakub Kicinski wrote:
> > If construction of the array of policies fails when recording
> > non-first policy we need to unwind.
> > 
> > Reported-by: syzbot+dc54d9ba8153b216cae0@syzkaller.appspotmail.com
> > Fixes: 50a896cf2d6f ("genetlink: properly support per-op policy dumping")
> > Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> > ---
> >  net/netlink/genetlink.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
> > index 1afca2a6c2ac..57010927e20a 100644
> > --- a/net/netlink/genetlink.c
> > +++ b/net/netlink/genetlink.c
> > @@ -1174,13 +1174,17 @@ static int ctrl_dumppolicy_start(struct netlink_callback *cb)
> >  							     op.policy,
> >  							     op.maxattr);
> >  			if (err)
> > -				return err;
> > +				goto err_free_state;  
> 
> There's another call to netlink_policy_dump_add_policy() right above
> this one. The patch I posted to syzkaller frees the memory inside
> netlink_policy_dump_add_policy() and the result was OK.

Oh, sorry, I didn't see this reply. Every single time after the merge
window is over the stable folks bombard the mailing systems with their
backported patches and bring the mailing lists to their knees :/

Do you see your posting on lore.kernel.org? I presume not. You really
need to use ./scripts/get_maintainer and CC relevant folks directly.

This is v2 of my fix I submitted to syzbot for testing:

https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git/commit/?h=genl-fix&id=66f01a660c4439fc78a6fc68413f895b8fd07474

The earlier call to netlink_policy_dump_add_policy() should not be 
a problem if the function unwound its state properly.