diff mbox series

[net] genetlink: Fix an error handling path in ctrl_dumppolicy_start()

Message ID 7186dae6d951495f6918c45f8250e6407d71e88f.1670878949.git.christophe.jaillet@wanadoo.fr
State New
Headers
Series [net] genetlink: Fix an error handling path in ctrl_dumppolicy_start() |

Commit Message

Christophe JAILLET Dec. 12, 2022, 9:03 p.m. UTC 
  If this memory allocation fails, some resources need to be freed.
Add the missing goto to the error handling path.

Fixes: b502b3185cd6 ("genetlink: use iterator in the op to policy map dumping")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
This patch is speculative.

This function is a callback and I don't know how the core works and handles
such situation, so review with care!

More-over, should this kmalloc() be a kzalloc()?
genl_op_iter_init() below does not initialize all fields, be they are maybe
set correctly before uses.
---
 net/netlink/genetlink.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Jakub Kicinski Dec. 12, 2022, 9:10 p.m. UTC | #1 
On Mon, 12 Dec 2022 22:03:06 +0100 Christophe JAILLET wrote:
> If this memory allocation fails, some resources need to be freed.
> Add the missing goto to the error handling path.
> 
> Fixes: b502b3185cd6 ("genetlink: use iterator in the op to policy map dumping")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> This patch is speculative.
> 
> This function is a callback and I don't know how the core works and handles
> such situation, so review with care!

It's fine, the function has pretty much two completely separate paths.
Dump all ops and dump a single op.
Anything that allocs state before this point is on the single op path,
while the iterator is only allocated for dump all.
This should be evident from the return 0; at the end of the 
  if (tb[CTRL_ATTR_OP])

> More-over, should this kmalloc() be a kzalloc()?
> genl_op_iter_init() below does not initialize all fields, be they are maybe
> set correctly before uses.

It's fine, op_iters are put on the stack without initializing, iter
init must (and currently does) work without depending on zeroed memory.
Jacob Keller Dec. 12, 2022, 9:23 p.m. UTC | #2 
> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Monday, December 12, 2022 1:10 PM
> To: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> Cc: David S. Miller <davem@davemloft.net>; Eric Dumazet
> <edumazet@google.com>; Paolo Abeni <pabeni@redhat.com>; Keller, Jacob E
> <jacob.e.keller@intel.com>; linux-kernel@vger.kernel.org; kernel-
> janitors@vger.kernel.org; netdev@vger.kernel.org
> Subject: Re: [PATCH net] genetlink: Fix an error handling path in
> ctrl_dumppolicy_start()
> 
> On Mon, 12 Dec 2022 22:03:06 +0100 Christophe JAILLET wrote:
> > If this memory allocation fails, some resources need to be freed.
> > Add the missing goto to the error handling path.
> >
> > Fixes: b502b3185cd6 ("genetlink: use iterator in the op to policy map dumping")
> > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> > ---
> > This patch is speculative.
> >
> > This function is a callback and I don't know how the core works and handles
> > such situation, so review with care!
> 
> It's fine, the function has pretty much two completely separate paths.
> Dump all ops and dump a single op.
> Anything that allocs state before this point is on the single op path,
> while the iterator is only allocated for dump all.
> This should be evident from the return 0; at the end of the
>   if (tb[CTRL_ATTR_OP])
> 
> > More-over, should this kmalloc() be a kzalloc()?
> > genl_op_iter_init() below does not initialize all fields, be they are maybe
> > set correctly before uses.

I personally prefer using kzalloc even if we know its not necessary, except in cases where performance of the allocation matters. It helps reduce the burden of review as one doesn't need to think "was this initialized?" at least for the problem of leaking kernel internals.

I know there are also some tools like UBSAN and others which might be able to detect access to uninitialized memory, but I am not sure if they're capable enough at present to handle memory returned by kmalloc or not. If they are, then there could be advantage in detecting cases where you did fully expect initialization to be done.

> 
> It's fine, op_iters are put on the stack without initializing, iter
> init must (and currently does) work without depending on zeroed memory.

The above said, I think the analysis here is correct and that kmalloc is ok here.

Thanks,
Jake
diff mbox series

Patch

diff --git a/net/netlink/genetlink.c b/net/netlink/genetlink.c
index 600993c80050..7b9f04bd85a2 100644
--- a/net/netlink/genetlink.c
+++ b/net/netlink/genetlink.c
@@ -1451,8 +1451,10 @@  static int ctrl_dumppolicy_start(struct netlink_callback *cb)
 	}
 
 	ctx->op_iter = kmalloc(sizeof(*ctx->op_iter), GFP_KERNEL);
-	if (!ctx->op_iter)
-		return -ENOMEM;
+	if (!ctx->op_iter) {
+		err = -ENOMEM;
+		goto err_free_state;
+	}
 
 	genl_op_iter_init(rt, ctx->op_iter);
 	ctx->dump_map = genl_op_iter_next(ctx->op_iter);