[PATCH iproute2] mptcp: avoid uninitialised errno usage

[PATCH iproute2] mptcp: avoid uninitialised errno usage

[Florian Westphal]

The function called *might* set errno based on errno value in NLMSG_ERROR
message, but in case no such message exists errno is left alone.

This may cause ip to fail with
    "can't subscribe to mptcp events: Success"

on kernels that support mptcp but lack event support (all kernels <= 5.11).

Set errno to a meaningful value.  This will then still exit with the
more specific 'permission denied' or some such when called by process
that lacks CAP_NET_ADMIN on 5.12 and later.

Fixes: ff619e4fd370 ("mptcp: add support for event monitoring")
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 ip/ipmptcp.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ip/ipmptcp.c b/ip/ipmptcp.c
index 5f490f0026d9..504b5b2f5329 100644
--- a/ip/ipmptcp.c
+++ b/ip/ipmptcp.c
@@ -491,6 +491,9 @@ out:
 
 static int mptcp_monitor(void)
 {
+	/* genl_add_mcast_grp may or may not set errno */
+	errno = EOPNOTSUPP;
+
 	if (genl_add_mcast_grp(&genl_rth, genl_family, MPTCP_PM_EV_GRP_NAME) < 0) {
 		perror("can't subscribe to mptcp events");
 		return 1;
-- 
2.26.3

Re: [PATCH iproute2] mptcp: avoid uninitialised errno usage

[David Ahern]

On 5/3/21 4:36 AM, Florian Westphal wrote:
> The function called *might* set errno based on errno value in NLMSG_ERROR
> message, but in case no such message exists errno is left alone.
> 
> This may cause ip to fail with
>     "can't subscribe to mptcp events: Success"
> 
> on kernels that support mptcp but lack event support (all kernels <= 5.11).
> 
> Set errno to a meaningful value.  This will then still exit with the
> more specific 'permission denied' or some such when called by process
> that lacks CAP_NET_ADMIN on 5.12 and later.
> 
> Fixes: ff619e4fd370 ("mptcp: add support for event monitoring")
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  ip/ipmptcp.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/ip/ipmptcp.c b/ip/ipmptcp.c
> index 5f490f0026d9..504b5b2f5329 100644
> --- a/ip/ipmptcp.c
> +++ b/ip/ipmptcp.c
> @@ -491,6 +491,9 @@ out:
>  
>  static int mptcp_monitor(void)
>  {
> +	/* genl_add_mcast_grp may or may not set errno */
> +	errno = EOPNOTSUPP;
> +
>  	if (genl_add_mcast_grp(&genl_rth, genl_family, MPTCP_PM_EV_GRP_NAME) < 0) {
>  		perror("can't subscribe to mptcp events");
>  		return 1;
> 

Seems like this should be set in genl_add_mcast_grp vs its caller.

Re: [PATCH iproute2] mptcp: avoid uninitialised errno usage

[Florian Westphal]

David Ahern <dsahern@gmail.com> wrote:
> On 5/3/21 4:36 AM, Florian Westphal wrote:
> > The function called *might* set errno based on errno value in NLMSG_ERROR
> > message, but in case no such message exists errno is left alone.
> > 
> > This may cause ip to fail with
> >     "can't subscribe to mptcp events: Success"
> > 
> > on kernels that support mptcp but lack event support (all kernels <= 5.11).
> > 
> > Set errno to a meaningful value.  This will then still exit with the
> > more specific 'permission denied' or some such when called by process
> > that lacks CAP_NET_ADMIN on 5.12 and later.
> > 
> > Fixes: ff619e4fd370 ("mptcp: add support for event monitoring")
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> >  ip/ipmptcp.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/ip/ipmptcp.c b/ip/ipmptcp.c
> > index 5f490f0026d9..504b5b2f5329 100644
> > --- a/ip/ipmptcp.c
> > +++ b/ip/ipmptcp.c
> > @@ -491,6 +491,9 @@ out:
> >  
> >  static int mptcp_monitor(void)
> >  {
> > +	/* genl_add_mcast_grp may or may not set errno */
> > +	errno = EOPNOTSUPP;
> > +
> >  	if (genl_add_mcast_grp(&genl_rth, genl_family, MPTCP_PM_EV_GRP_NAME) < 0) {
> >  		perror("can't subscribe to mptcp events");
> >  		return 1;
> > 
> 
> Seems like this should be set in genl_add_mcast_grp vs its caller.

I think setting errno in libraries (libc excluded) is a bad design
choice.  If you still disagree, then I can respin, but it would get a
bit more ugly, e.g. (untested!):

diff --git a/lib/libgenl.c b/lib/libgenl.c
--- a/lib/libgenl.c
+++ b/lib/libgenl.c
@@ -100,20 +100,29 @@ int genl_add_mcast_grp(struct rtnl_handle *grth, __u16 fnum, const char *group)
 
 	addattr16(&req.n, sizeof(req), CTRL_ATTR_FAMILY_ID, fnum);
 
+	/* clear errno to set a default value if needed */
+	errno = 0;
+
 	if (rtnl_talk(grth, &req.n, &answer) < 0) {
 		fprintf(stderr, "Error talking to the kernel\n");
+		if (errno == 0)
+			errno = EOPNOTSUPP;
 		return -2;
 	}
 
 	ghdr = NLMSG_DATA(answer);
 	len = answer->nlmsg_len;
 
-	if (answer->nlmsg_type != GENL_ID_CTRL)
+	if (answer->nlmsg_type != GENL_ID_CTRL) {
+		errno = EINVAL;
 		goto err_free;
+	}
 
 	len -= NLMSG_LENGTH(GENL_HDRLEN);
-	if (len < 0)
+	if (len < 0) {
+		errno = EINVAL;
 		goto err_free;
+	}
 
 	attrs = (struct rtattr *) ((char *) ghdr + GENL_HDRLEN);
 	parse_rtattr(tb, CTRL_ATTR_MAX, attrs, len);
@@ -130,6 +139,10 @@ int genl_add_mcast_grp(struct rtnl_handle *grth, __u16 fnum, const char *group)
 
 err_free:
 	free(answer);
+
+	if (ret < 0 && errno == 0)
+		errno = EOPNOTSUPP;
+
 	return ret;
 }
 

Re: [PATCH iproute2] mptcp: avoid uninitialised errno usage

[David Ahern]

On 5/9/21 4:25 PM, Florian Westphal wrote:
> David Ahern <dsahern@gmail.com> wrote:
>> On 5/3/21 4:36 AM, Florian Westphal wrote:
>>> The function called *might* set errno based on errno value in NLMSG_ERROR
>>> message, but in case no such message exists errno is left alone.
>>>
>>> This may cause ip to fail with
>>>     "can't subscribe to mptcp events: Success"
>>>
>>> on kernels that support mptcp but lack event support (all kernels <= 5.11).
>>>
>>> Set errno to a meaningful value.  This will then still exit with the
>>> more specific 'permission denied' or some such when called by process
>>> that lacks CAP_NET_ADMIN on 5.12 and later.
>>>
>>> Fixes: ff619e4fd370 ("mptcp: add support for event monitoring")
>>> Signed-off-by: Florian Westphal <fw@strlen.de>
>>> ---
>>>  ip/ipmptcp.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/ip/ipmptcp.c b/ip/ipmptcp.c
>>> index 5f490f0026d9..504b5b2f5329 100644
>>> --- a/ip/ipmptcp.c
>>> +++ b/ip/ipmptcp.c
>>> @@ -491,6 +491,9 @@ out:
>>>  
>>>  static int mptcp_monitor(void)
>>>  {
>>> +	/* genl_add_mcast_grp may or may not set errno */
>>> +	errno = EOPNOTSUPP;
>>> +
>>>  	if (genl_add_mcast_grp(&genl_rth, genl_family, MPTCP_PM_EV_GRP_NAME) < 0) {
>>>  		perror("can't subscribe to mptcp events");
>>>  		return 1;
>>>
>>
>> Seems like this should be set in genl_add_mcast_grp vs its caller.
> 
> I think setting errno in libraries (libc excluded) is a bad design

lib/libnetlink.c, rtnl_talk for example already does. I think it would
be best for the location of the error to set the errno.

Your suggested change here goes way beyond setting a default errno
before calling genl_add_mcast_grp.

> choice.  If you still disagree, then I can respin, but it would get a
> bit more ugly, e.g. (untested!):
> 
> diff --git a/lib/libgenl.c b/lib/libgenl.c
> --- a/lib/libgenl.c
> +++ b/lib/libgenl.c
> @@ -100,20 +100,29 @@ int genl_add_mcast_grp(struct rtnl_handle *grth, __u16 fnum, const char *group)
>  
>  	addattr16(&req.n, sizeof(req), CTRL_ATTR_FAMILY_ID, fnum);
>  
> +	/* clear errno to set a default value if needed */
> +	errno = 0;
> +
>  	if (rtnl_talk(grth, &req.n, &answer) < 0) {
>  		fprintf(stderr, "Error talking to the kernel\n");
> +		if (errno == 0)
> +			errno = EOPNOTSUPP;

you don't list the above string in the output in the commit log. Staring
at rtnl_talk and recvmsg and its failure paths, it seems unlikely that
path is causing the problem.

>  		return -2;
>  	}
>  
>  	ghdr = NLMSG_DATA(answer);
>  	len = answer->nlmsg_len;
>  
> -	if (answer->nlmsg_type != GENL_ID_CTRL)
> +	if (answer->nlmsg_type != GENL_ID_CTRL) {
> +		errno = EINVAL;
>  		goto err_free;
> +	}
>  
>  	len -= NLMSG_LENGTH(GENL_HDRLEN);
> -	if (len < 0)
> +	if (len < 0) {
> +		errno = EINVAL;
>  		goto err_free;
> +	}

EINVAL here is different than what you have in the commit message. Are
one of these the location of the real problem? Seems likely since your
commit log only showed "can't subscribe to mptcp events: Success" and
not any other error strings.

e.g., if CTRL_ATTR_MCAST_GROUPS is NULL, that would be the place to put
the EOPNOTSUPP, but then it too has a string not listed in your commit log.

>  
>  	attrs = (struct rtattr *) ((char *) ghdr + GENL_HDRLEN);
>  	parse_rtattr(tb, CTRL_ATTR_MAX, attrs, len);
> @@ -130,6 +139,10 @@ int genl_add_mcast_grp(struct rtnl_handle *grth, __u16 fnum, const char *group)
>  
>  err_free:
>  	free(answer);
> +
> +	if (ret < 0 && errno == 0)
> +		errno = EOPNOTSUPP;
> +
>  	return ret;
>  }
>  
> 

Re: [PATCH iproute2] mptcp: avoid uninitialised errno usage

[Florian Westphal]

David Ahern <dsahern@gmail.com> wrote:
> >  	if (rtnl_talk(grth, &req.n, &answer) < 0) {
> >  		fprintf(stderr, "Error talking to the kernel\n");
> > +		if (errno == 0)
> > +			errno = EOPNOTSUPP;
> 
> you don't list the above string in the output in the commit log. Staring
> at rtnl_talk and recvmsg and its failure paths, it seems unlikely that
> path is causing the problem.

Its not in my particular case, but if it would caller would still get random errno.

The sketch I sent merely provides a relible errno whenever ret is less
than 0.  Right now it may or may not have been set.

Re: [PATCH iproute2] mptcp: avoid uninitialised errno usage

[David Ahern]

On 5/9/21 4:55 PM, Florian Westphal wrote:
> David Ahern <dsahern@gmail.com> wrote:
>>>  	if (rtnl_talk(grth, &req.n, &answer) < 0) {
>>>  		fprintf(stderr, "Error talking to the kernel\n");
>>> +		if (errno == 0)
>>> +			errno = EOPNOTSUPP;
>>
>> you don't list the above string in the output in the commit log. Staring
>> at rtnl_talk and recvmsg and its failure paths, it seems unlikely that
>> path is causing the problem.
> 
> Its not in my particular case, but if it would caller would still get random errno.
> 
> The sketch I sent merely provides a relible errno whenever ret is less
> than 0.  Right now it may or may not have been set.
> 

Then let's find and fix those locations.

Unless I missed a code path, rtnl_talk and friends (e.g.,
__rtnl_talk_iov) all set errno before any < 0 return.