Re: [PATCH] nbd: Fix memory leak in nbd_add_socket

[Eric Biggers <ebiggers@kernel.org>]

On Thu, Jun 25, 2020 at 12:16:33AM +0000, Chaitanya Kulkarni wrote:
> On 6/12/20 1:57 AM, Zheng Bin wrote:
> > nbd_add_socket
> >    socks = krealloc(num_connections+1) -->if num_connections is 0, alloc 1
> >    nsock = kzalloc                     -->If fail, will return
> > 
> > nbd_config_put
> >    if (config->num_connections)        -->0, not free
> >      kfree(config->socks)
> > 
> > Thus memleak happens, this patch fixes that.
> > 
> > Signed-off-by: Zheng Bin<zhengbin13@huawei.com>

This appears to address the syzbot report "memory leak in nbd_add_socket"
https://syzkaller.appspot.com/bug?id=08283193956ab772623e65242b3ed6d0e7c7d9ce
Can you please add:

	Reported-by: syzbot+934037347002901b8d2a@syzkaller.appspotmail.com

> 
> Not an nbd expert but wouldn't it be easier use following which matches 
> the + 1 in the nbd_add_socket() :-
> 
> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> index 01794cd2b6ca..e67c790039c9 100644
> --- a/drivers/block/nbd.c
> +++ b/drivers/block/nbd.c
> @@ -1209,9 +1209,9 @@ static void nbd_config_put(struct nbd_device *nbd)
>                          device_remove_file(disk_to_dev(nbd->disk), 
> &pid_attr);
>                  nbd->task_recv = NULL;
>                  nbd_clear_sock(nbd);
> -               if (config->num_connections) {
> +               if (config->num_connections + 1) {
>                          int i;
> -                       for (i = 0; i < config->num_connections; i++) {
> +                       for (i = 0; i < (config->num_connections + 1); 
> i++) {
>                                  sockfd_put(config->socks[i]->sock);
>                                  kfree(config->socks[i]);
>                          }

That looks wrong.  The + 1 is just nbd_add_socket() preparing to append an entry
to the array.

Why not just check whether the pointer is NULL or not:

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 43cff01a5a67..cb8e86174edb 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -1206,7 +1206,7 @@ static void nbd_config_put(struct nbd_device *nbd)
 			device_remove_file(disk_to_dev(nbd->disk), &pid_attr);
 		nbd->task_recv = NULL;
 		nbd_clear_sock(nbd);
-		if (config->num_connections) {
+		if (config->socks) {
 			int i;
 			for (i = 0; i < config->num_connections; i++) {
 				sockfd_put(config->socks[i]->sock);