linux-kernel - Re: [PATCH net-next] vxlan: Return the correct errno code

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 07 Jun 2021 07:50:19 -0700
From:   Joe Perches <joe@...ches.com>
To:     Zheng Yongjun <zhengyongjun3@...wei.com>, davem@...emloft.net,
        kuba@...nel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] vxlan: Return the correct errno code

On Mon, 2021-06-07 at 22:44 +0800, Zheng Yongjun wrote:
> When kalloc or kmemdup failed, should return ENOMEM rather than ENOBUFS.

Why?  Where in the call chain does it matter?
Have you inspected the entire call chain and their return value tests?

> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
[]
> @@ -711,11 +711,11 @@ static int vxlan_fdb_append(struct vxlan_fdb *f,
>  
> 
>  	rd = kmalloc(sizeof(*rd), GFP_ATOMIC);

And this should probably use kzalloc to avoid possible uninitialized members.

>  	if (rd == NULL)
> -		return -ENOBUFS;
> +		return -ENOMEM;
>  
>  	if (dst_cache_init(&rd->dst_cache, GFP_ATOMIC)) {
>  		kfree(rd);
> -		return -ENOBUFS;
> +		return -ENOMEM;
>  	}
>  
>  	rd->remote_ip = *ip;

The struct is:

include/net/vxlan.h:struct vxlan_rdst {
include/net/vxlan.h-    union vxlan_addr         remote_ip;
include/net/vxlan.h-    __be16                   remote_port;
include/net/vxlan.h-    u8                       offloaded:1;
include/net/vxlan.h-    __be32                   remote_vni;
include/net/vxlan.h-    u32                      remote_ifindex;
include/net/vxlan.h-    struct net_device        *remote_dev;
include/net/vxlan.h-    struct list_head         list;
include/net/vxlan.h-    struct rcu_head          rcu;
include/net/vxlan.h-    struct dst_cache         dst_cache;
include/net/vxlan.h-};

And the code is:

	if (dst_cache_init(&rd->dst_cache, GFP_ATOMIC)) {
		kfree(rd);
		return -ENOBUFS;
	}

	rd->remote_ip = *ip;
	rd->remote_port = port;
	rd->offloaded = false;
	rd->remote_vni = vni;
	rd->remote_ifindex = ifindex;

	list_add_tail_rcu(&rd->list, &f->remotes);

	*rdp = rd;

So it appears as if rd->remote_dev and rd->rcu are uninitialized.
I don't know if that matters, but it seems poor form.