net: context: connect: Make sure local end is bound before connecting #785

pfalcon · 2017-07-13T20:39:48Z

Introduce net_context_bind_default() to ensure that local address is
set for context if not yet (via explict bind() call). This fixes
dereferences of NULL pointer to local address which was exposed when
MMU was enabled for qemu_x86.

Signed-off-by: Paul Sokolovsky paul.sokolovsky@linaro.org

pfalcon · 2017-07-13T20:40:54Z

Proposed fix for #752 (comment) . If approved, call to net_context_bind_default() might be added to some other functions (in separate patches).

jukkar · 2017-07-14T06:28:13Z

subsys/net/ip/net_context.c

+		return -EADDRNOTAVAIL;
+	}
+
+	if (dest_addr->family == AF_INET6) {


I think we should put

#if defined(CONFIG_NET_IPV{6|4}) ... #endif /* ... */

checks around the family checkings, so similar way as what is done in other functions.

Yup, fixed.

jukkar · 2017-07-14T06:29:44Z

subsys/net/ip/net_context.c

+static int net_context_bind_default(struct net_context *context,
+				    const struct sockaddr *dest_addr)
+{
+	struct net_if *iface = net_if_get_default();


If the interface is already set for the context, then this should not be done. If the app / other part of the stack has already set the interface, this code now overwrites that value.

I may imagine you're commented on the wrong line. Anyway, two points: 1) This code sets the local address, which is bound to an interface, so if it sets address, it also sets interface. 2) I spent reasonable time to look into code, and on "outgoing" path, only bind() sets local address and iface (all other paths appear to be for incoming packets/connections). 3) Final point obvious from code is that iface is not set if address is already set, because p.1 (they always set together).

jukkar · 2017-07-14T06:32:14Z

subsys/net/ip/net_context.c

+			(struct in6_addr *)net_ipv6_unspecified_address();
+
+		return 0;
+	} else if (dest_addr->family == AF_INET) {


There should be no need for "else" here as if we enter the ipv6 branch, we always return from it. This way it is cleaner to put the pre-processor if's around this block.

jukkar · 2017-07-14T06:33:05Z

subsys/net/ip/net_context.c

@@ -689,6 +689,45 @@ int net_context_put(struct net_context *context)
 	return 0;
 }

+static int net_context_bind_default(struct net_context *context,


As this is internal function, perhaps calling this bind_default() would be ok.

jukkar · 2017-07-14T06:34:20Z

subsys/net/ip/net_context.c

+
+	if (!iface) {
+		NET_ERR("Cannot bind to %s",
+			net_sprint_ipv6_addr(&addr6->sin6_addr));


Isn't there compilation error here if debugging is enabled as I do not see add6 defined anywhere? And we bind to any address later in this function so this debug print would give wrong info anyway.

Copy-paste error, will remove refs to ipv6.

jukkar · 2017-07-14T06:42:50Z

subsys/net/ip/net_context.c

+		NET_ERR("Cannot bind to %s",
+			net_sprint_ipv6_addr(&addr6->sin6_addr));
+
+		return -EADDRNOTAVAIL;


The interface checking needs to be done differently, perhaps something like this

if (!net_context_get_iface(context)) { net_context_set_iface(context, net_if_get_default()); if (!net_context_get_iface(context)) { return -ENOENT; } }

although I am not sure if we really need to check if the default interface returned NULL, probably NET_ASSERT() would be sufficient here.

Sorry, this code copied from net_context_bind(). If you think checking needs to be done differently, then net_context_bind() needs to do it, but I don't see anything wrong with what it does. Again, bind_default() function performs the same operation as bind(.., INADDR_ANY, ...), the reason I don't use bind() directly is because it requires munging with address families to use particular INADDR_ANY impl for each. (bind_default also checks before binds.)

jukkar · 2017-07-14T06:45:51Z

subsys/net/ip/net_context.c

+		net_sin6_ptr(&context->local)->sin6_family = AF_INET6;
+		net_sin6_ptr(&context->local)->sin6_addr =
+			(struct in6_addr *)net_ipv6_unspecified_address();
+


We need to check the port number here too. The net_context_get() sets the initial value for the port, but it might be used already when we get into this point. So we need to check it by calling check_used_port() like the net_context_bind() is doing, and probably call find_available_port() again if the port was used already (unlikely to happen thou).

Let's treat this as a separate issue. So far, I indeed rely that random port was set at the time of context creation. As you say, that's actually too early, and may be already used at the time of actual network operation. But the obvious solution is to move setting random port to bind_default(), which is a separate patch, requiring separate consideration and testing (UDP cases, etc.)

Fair enough, lets deal with the port issue in later patches.

jukkar · 2017-07-14T06:51:02Z

subsys/net/ip/net_context.c

@@ -689,6 +689,45 @@ int net_context_put(struct net_context *context)
 	return 0;
 }

+static int net_context_bind_default(struct net_context *context,
+				    const struct sockaddr *dest_addr)


Having dest_addr here is great, but the function is using it only for family currently. So the code is missing a call to

src_addr = net_if_ipv6_select_src_addr(NULL, dest_addr);

and then the src_addr can be used to get the interface by calling

if (!net_if_ipv6_addr_lookup(src_addr, &iface)) { return -EINVAL; }

If the caller does not supply the dest_addr, then we can use the default interface. For IPv4 there is no source address selection function currently, so we can just use the default interface for IPv4.

Actually realized that the dest_addr cannot be null as then we would not know the family.

Yes, dest_addr is passed so we can select address family. My initial idea was just to a special case to existing net_context_bind(), when addr=NULL is passed to it, perform this default binding. But then we wouldn't know with AF to select indeed. So instead, I factored out this logic into separate function, which accepts dest address. It's called from connect(), where destaddr is known, and I may imagine it may need to be called form recv(), send() on UDP path, where destaddr is also known.

So, that's the reason why this function is written like it is - it mimics net_context_bind(). Note that's not the only way to select AF, because context is created with specific AF ;-). But doing it like bind() does kinda leave possibility open to have Linux-style dual-family sockets.

jukkar · 2017-07-14T07:10:13Z

BTW, we can shorten the commit subject a bit, so "net: context: ..." is enough, no need to put the ": connect" there.

pfalcon · 2017-07-14T10:10:19Z

BTW, we can shorten the commit subject a bit, so "net: context: ..." is enough, no need to put the ": connect" there.

Well, it fits within limit, and I'm proponent on commit subjects which give more info, not less (e.g., as I mentioned above, similar change may need to be applied to send()/recv()).

pfalcon · 2017-07-14T12:37:55Z

Ok, fixes applied and shippable passes. @jukkar : Please re-review.

jukkar · 2017-07-17T07:24:24Z

subsys/net/ip/net_context.c

@@ -689,6 +689,49 @@ int net_context_put(struct net_context *context)
 	return 0;
 }

+static int bind_default(struct net_context *context,
+				    const struct sockaddr *dest_addr)


Indentation went wrong after the function name was shortened.

jukkar · 2017-07-17T07:27:35Z

subsys/net/ip/net_context.c

+static int bind_default(struct net_context *context,
+				    const struct sockaddr *dest_addr)
+{
+	struct net_if *iface = net_if_get_default();


If we have multiple interfaces, then this selection might get wrong interface.
If the user has supplied a real destination address (and not just unspecified address), then we would need to select the interface according to that dest address. Only if the user does not set the dest address, then we can use the default interface. I wrote a small piece of code for that in my earlier review.

If we have multiple interfaces, then this selection might get wrong interface.

Yes, it might. Just the same as net_context_bind(). My assumption is that if explicit bind() is not called, it's called implicitly as bind(INADDR_ANY, rand()). bind_default() just performs the same operations as next_context_bind(INADDR_ANY, rand()), using the same code (the reason I factored out code into new function instead of just calling next_context_bind() because it seemed like a chore to construct INADDR_ANY for IPv4 vs IPv6 cases; maybe that was wrong actually).

I can paste your code in, but the only expected outcome from that would be that implementation (and thus behavior) of explicit vs implicit bind diverge, and thus lead to confusion. Note that neither next_context_bind() nor your proposed change to bind_default() is correct impl of INADDR_ANY - per POSIX, it should bind to all not to default/"best" interface.

jukkar · 2017-07-17T07:29:16Z

subsys/net/ip/net_context.c

+		net_sin6_ptr(&context->local)->sin6_family = AF_INET6;
+		net_sin6_ptr(&context->local)->sin6_addr =
+			(struct in6_addr *)net_ipv6_unspecified_address();
+


Fair enough, lets deal with the port issue in later patches.

Introduce net_context_bind_default() to ensure that local address is set for context if not yet (via explict bind() call). This fixes dereferences of NULL pointer to local address which was exposed when MMU was enabled for qemu_x86. Signed-off-by: Paul Sokolovsky <paul.sokolovsky@linaro.org>

pfalcon · 2017-07-17T10:53:23Z

Closed in favor of #817

pfalcon requested a review from jukkar July 13, 2017 20:39

jukkar requested changes Jul 14, 2017

View reviewed changes

jukkar added the net label Jul 14, 2017

pfalcon force-pushed the net-default-bind branch 2 times, most recently from 7079640 to 1be5ec9 Compare July 14, 2017 11:09

jukkar requested changes Jul 17, 2017

View reviewed changes

pfalcon force-pushed the net-default-bind branch from 1be5ec9 to b8bf80c Compare July 17, 2017 08:46

pfalcon mentioned this pull request Jul 17, 2017

net: context: connect: Make sure local end is bound before connecting #817

Merged

pfalcon closed this Jul 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: context: connect: Make sure local end is bound before connecting #785

net: context: connect: Make sure local end is bound before connecting #785

pfalcon commented Jul 13, 2017

pfalcon commented Jul 13, 2017

jukkar Jul 14, 2017

pfalcon Jul 14, 2017

jukkar Jul 14, 2017

pfalcon Jul 14, 2017

jukkar Jul 14, 2017

pfalcon Jul 14, 2017

jukkar Jul 14, 2017

pfalcon Jul 14, 2017

jukkar Jul 14, 2017

pfalcon Jul 14, 2017

jukkar Jul 14, 2017

pfalcon Jul 14, 2017

jukkar Jul 14, 2017 •

edited

pfalcon Jul 14, 2017

jukkar Jul 17, 2017

jukkar Jul 14, 2017 •

edited

jukkar Jul 14, 2017

pfalcon Jul 14, 2017

jukkar commented Jul 14, 2017

pfalcon commented Jul 14, 2017

pfalcon commented Jul 14, 2017

jukkar Jul 17, 2017

pfalcon Jul 17, 2017

jukkar Jul 17, 2017

pfalcon Jul 17, 2017

jukkar Jul 17, 2017

pfalcon commented Jul 17, 2017

net: context: connect: Make sure local end is bound before connecting #785

net: context: connect: Make sure local end is bound before connecting #785

Conversation

pfalcon commented Jul 13, 2017

pfalcon commented Jul 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jukkar Jul 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jukkar Jul 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jukkar commented Jul 14, 2017

pfalcon commented Jul 14, 2017

pfalcon commented Jul 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pfalcon commented Jul 17, 2017

jukkar Jul 14, 2017 •

edited

jukkar Jul 14, 2017 •

edited