netdev - Re: [PATCH net-next RFC] ipv4 neigh: match add/del dev lookup behaviour to SIOCSARP/SIOCDARP

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <54EC8C23.5060204@cumulusnetworks.com>
Date:	Tue, 24 Feb 2015 06:35:15 -0800
From:	roopa <roopa@...ulusnetworks.com>
To:	"Rosen, Rami" <rami.rosen@...el.com>
CC:	"hadi@...atatu.com" <hadi@...atatu.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"hannes@...essinduktion.org" <hannes@...essinduktion.org>
Subject: Re: [PATCH net-next RFC] ipv4 neigh: match add/del dev lookup behaviour
 to SIOCSARP/SIOCDARP

On 2/24/15, 6:01 AM, Rosen, Rami wrote:
> Hi,
>
> @@ -1692,24 +1703,31 @@ static int neigh_add(struct sk_buff *skb, struct nlmsghdr *nlh)
>   		goto out;
>   
>   	ndm = nlmsg_data(nlh);
> +	tbl = neigh_find_table(ndm->ndm_family);
> +	if (tbl == NULL)
> +		return -EAFNOSUPPORT;
> +
> +	if (nla_len(tb[NDA_DST]) < tbl->key_len)
> +		goto out;
> +
> +	dst = nla_data(tb[NDA_DST]);
>   	if (ndm->ndm_ifindex) {
>   		dev = __dev_get_by_index(net, ndm->ndm_ifindex);
>   		if (dev == NULL) {
>   			err = -ENODEV;
>   			goto out;
>   		}
>
> If ip_route_output() fails, then the neigh_add() method will return -EINVAL (as it was initialized thus);
I was intentionally trying not to return any special error code if 
ip_route_output() fails. So the patch tries to make the ip_route_output 
failure same as the case where the user did not specify a device.

> or, in cases when the NTF_PROXY flag is set, it can return 0 or ENOBUFS, according to the result of the pneigh_lookup(), which is called immediately subsequently in this path. But in fact __ip_route_output_key(), which is invoked by ip_route_output(), can return different error codes, like -EINVAL,-ENODEV, -ENETUNREACH and more, via ERR_PTR.
>
>   So maybe better is (in order to reflect the cause of the error):
> +		if (!IS_ERR(rt)) {
> +			dev = rt->dst.dev;
> +			ip_rt_put(rt);
> +		} else
>                                                 return PTR_ERR(rt);

I intentionally don't return here ..because from the original code, the 
NTF_PROXY case seemed to be ok with the device being NULL.  The device 
NULL check was after the NTF_PROXY code.

>
> We can exit the method at this point with "return PTR_ERR(rt)" because we are assured at this point that dev is NULL.
>
> Error handling in neigh_delete() method maybe also be better changed accordingly.

I intend to do some testing for the NTF_PROXY case, however, AFAICT, 
this patch should not have introduced any functional changes in that path.

Thanks!.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html