Endless loop in netlink_msg_to_ifaddr

Endless loop in netlink_msg_to_ifaddr

[Matej Kupljen]

[-- Attachment #1: Type: text/plain, Size: 1799 bytes --]

Hi all,

I am using OpenWRT device with MUSL C library version 1.1.19 and I am
running custom binary on it. I noticed that during testing my program
started using 99% CPU.
I build OpenWRT myself so I have all the sources. I attached the gdbserver
and checked what is going on.

What I found is that it executes this code:

static int netlink_msg_to_ifaddr(void *pctx, struct nlmsghdr *h)
{
struct ifaddrs_ctx *ctx = pctx;
struct ifaddrs_storage *ifs, *ifs0;
struct ifinfomsg *ifi = NLMSG_DATA(h);
struct ifaddrmsg *ifa = NLMSG_DATA(h);
struct rtattr *rta;
int stats_len = 0;

if (h->nlmsg_type == RTM_NEWLINK) {
for (rta = NLMSG_RTA(h, sizeof(*ifi)); NLMSG_RTAOK(rta, h); rta =
RTA_NEXT(rta)) {
if (rta->rta_type != IFLA_STATS) continue;
stats_len = RTA_DATALEN(rta);
break;
}
} else {
for (ifs0 = ctx->hash[ifa->ifa_index % IFADDRS_HASH_SIZE]; ifs0; ifs0 =
ifs0->hash_next)
if (ifs0->index == ifa->ifa_index)
break;
if (!ifs0) return 0;
}

From my GDB session:
(gdb) p *ctx
$10 = {first = 0xe1b0b0, last = 0xe1b1c0, hash = {0x0, 0xe1b0b0, 0xe1b1c0,
0x0 <repeats 61 times>}}
(gdb) p *h
$11 = {nlmsg_len = 1496, nlmsg_type = 16, nlmsg_flags = 2, nlmsg_seq = 1,
nlmsg_pid = 3143810850}
(gdb) p *ifi
$12 = {ifi_family = 0 '\000', __ifi_pad = 0 '\000', ifi_type = 1, ifi_index
= 3, ifi_flags = 69699, ifi_change = 0}
(gdb) p *ifa
$13 = {ifa_family = 0 '\000', ifa_prefixlen = 0 '\000', ifa_flags = 0
'\000', ifa_scope = 1 '\001', ifa_index = 3
(gdb) p *rta
$14 = {rta_len = 0, rta_type = 41}

As you can see the first message in netlink reply has a rta_len set to zero
so the list is never traversed, only the first message is received every
time.

I am not sure if this is the correct response from netlink, however the
program is stucked here.

Any ideas?
Please CC me in reply.

Thanks and BR,
Matej

[-- Attachment #2: Type: text/html, Size: 3158 bytes --]

Re: Endless loop in netlink_msg_to_ifaddr

[Timo Teras]

On Wed, 30 May 2018 11:57:03 +0200
Matej Kupljen <matej.kupljen@gmail.com> wrote:

> I am using OpenWRT device with MUSL C library version 1.1.19 and I am
> running custom binary on it. I noticed that during testing my program
> started using 99% CPU.
> I build OpenWRT myself so I have all the sources. I attached the
> gdbserver and checked what is going on.

Thanks for the report!

> As you can see the first message in netlink reply has a rta_len set
> to zero so the list is never traversed, only the first message is
> received every time.
> 
> I am not sure if this is the correct response from netlink, however
> the program is stucked here.
>
> Any ideas?
> Please CC me in reply.

That is invalid message to my understanding. Perhaps there's some new
extensions that allow it. Upstream (linux kernel) RTA_OK does do
additional checks against this situation.

The same issue probably affects if_nameindex.

I think the following should fix it:

diff --git a/src/network/netlink.h b/src/network/netlink.h
index 20700ac5..00dc7172 100644
--- a/src/network/netlink.h
+++ b/src/network/netlink.h
@@ -80,13 +80,13 @@ struct ifaddrmsg {
 #define NLMSG_DATALEN(nlh)	((nlh)->nlmsg_len-sizeof(struct nlmsghdr))
 #define NLMSG_DATAEND(nlh)	((char*)(nlh)+(nlh)->nlmsg_len)
 #define NLMSG_NEXT(nlh)		(struct nlmsghdr*)((char*)(nlh)+NETLINK_ALIGN((nlh)->nlmsg_len))
-#define NLMSG_OK(nlh,end)	((char*)(end)-(char*)(nlh) >= sizeof(struct nlmsghdr))
+#define NLMSG_OK(nlh,end)	((char*)(end)-(char*)(nlh) >= sizeof(struct nlmsghdr) && (nlh)->nlmsg_len >= sizeof(struct nlmsghdr))
 
 #define RTA_DATA(rta)		((void*)((char*)(rta)+sizeof(struct rtattr)))
 #define RTA_DATALEN(rta)	((rta)->rta_len-sizeof(struct rtattr))
 #define RTA_DATAEND(rta)	((char*)(rta)+(rta)->rta_len)
 #define RTA_NEXT(rta)		(struct rtattr*)((char*)(rta)+NETLINK_ALIGN((rta)->rta_len))
-#define RTA_OK(nlh,end)		((char*)(end)-(char*)(rta) >= sizeof(struct rtattr))
+#define RTA_OK(rta,end)		((char*)(end)-(char*)(rta) >= sizeof(struct rtattr) && (rta)->rta_len >= sizeof(struct rtattr))
 
 #define NLMSG_RTA(nlh,len)	((void*)((char*)(nlh)+sizeof(struct nlmsghdr)+NETLINK_ALIGN(len)))
 #define NLMSG_RTAOK(rta,nlh)	RTA_OK(rta,NLMSG_DATAEND(nlh))


Could you try if this fixes it?

You will probably need to 'make clean' or at least force recompilation
of src/network/{getifaddrs,if_nameindex,netlink}.c as the netlink.h
dependency is not picked up by the makefile automatically.

@dalias, if the above looks good to you, I am happy to submit properly
formatted git patch for it.

Thanks,
Timo

Re: Endless loop in netlink_msg_to_ifaddr

[Rich Felker]

On Wed, May 30, 2018 at 03:45:29PM +0300, Timo Teras wrote:
> On Wed, 30 May 2018 11:57:03 +0200
> Matej Kupljen <matej.kupljen@gmail.com> wrote:
> 
> > I am using OpenWRT device with MUSL C library version 1.1.19 and I am
> > running custom binary on it. I noticed that during testing my program
> > started using 99% CPU.
> > I build OpenWRT myself so I have all the sources. I attached the
> > gdbserver and checked what is going on.
> 
> Thanks for the report!
> 
> > As you can see the first message in netlink reply has a rta_len set
> > to zero so the list is never traversed, only the first message is
> > received every time.
> > 
> > I am not sure if this is the correct response from netlink, however
> > the program is stucked here.
> >
> > Any ideas?
> > Please CC me in reply.
> 
> That is invalid message to my understanding. Perhaps there's some new
> extensions that allow it. Upstream (linux kernel) RTA_OK does do
> additional checks against this situation.
> 
> The same issue probably affects if_nameindex.
> 
> I think the following should fix it:
> 
> diff --git a/src/network/netlink.h b/src/network/netlink.h
> index 20700ac5..00dc7172 100644
> --- a/src/network/netlink.h
> +++ b/src/network/netlink.h
> @@ -80,13 +80,13 @@ struct ifaddrmsg {
>  #define NLMSG_DATALEN(nlh)	((nlh)->nlmsg_len-sizeof(struct nlmsghdr))
>  #define NLMSG_DATAEND(nlh)	((char*)(nlh)+(nlh)->nlmsg_len)
>  #define NLMSG_NEXT(nlh)		(struct nlmsghdr*)((char*)(nlh)+NETLINK_ALIGN((nlh)->nlmsg_len))
> -#define NLMSG_OK(nlh,end)	((char*)(end)-(char*)(nlh) >= sizeof(struct nlmsghdr))
> +#define NLMSG_OK(nlh,end)	((char*)(end)-(char*)(nlh) >= sizeof(struct nlmsghdr) && (nlh)->nlmsg_len >= sizeof(struct nlmsghdr))
>  
>  #define RTA_DATA(rta)		((void*)((char*)(rta)+sizeof(struct rtattr)))
>  #define RTA_DATALEN(rta)	((rta)->rta_len-sizeof(struct rtattr))
>  #define RTA_DATAEND(rta)	((char*)(rta)+(rta)->rta_len)
>  #define RTA_NEXT(rta)		(struct rtattr*)((char*)(rta)+NETLINK_ALIGN((rta)->rta_len))
> -#define RTA_OK(nlh,end)		((char*)(end)-(char*)(rta) >= sizeof(struct rtattr))
> +#define RTA_OK(rta,end)		((char*)(end)-(char*)(rta) >= sizeof(struct rtattr) && (rta)->rta_len >= sizeof(struct rtattr))
>  
>  #define NLMSG_RTA(nlh,len)	((void*)((char*)(nlh)+sizeof(struct nlmsghdr)+NETLINK_ALIGN(len)))
>  #define NLMSG_RTAOK(rta,nlh)	RTA_OK(rta,NLMSG_DATAEND(nlh))
> 
> 
> Could you try if this fixes it?

I'm still waiting to hear whether this fixed it.

> You will probably need to 'make clean' or at least force recompilation
> of src/network/{getifaddrs,if_nameindex,netlink}.c as the netlink.h
> dependency is not picked up by the makefile automatically.
> 
> @dalias, if the above looks good to you, I am happy to submit properly
> formatted git patch for it.

I don't see anything obvious wrong with the proposed patch, but it
would be nice to have a better understanding of why it's needed and
whether this is a workaround for a kernel bug (present in which
kernels?) or something else.

Rich

Re: Endless loop in netlink_msg_to_ifaddr

[Andrei Vagin]

On Fri, Jun 01, 2018 at 09:44:33PM -0400, Rich Felker wrote:
> On Wed, May 30, 2018 at 03:45:29PM +0300, Timo Teras wrote:
> > On Wed, 30 May 2018 11:57:03 +0200
> > Matej Kupljen <matej.kupljen@gmail.com> wrote:
> > 
> > > I am using OpenWRT device with MUSL C library version 1.1.19 and I am
> > > running custom binary on it. I noticed that during testing my program
> > > started using 99% CPU.
> > > I build OpenWRT myself so I have all the sources. I attached the
> > > gdbserver and checked what is going on.
> > 
> > Thanks for the report!
> > 
> > > As you can see the first message in netlink reply has a rta_len set
> > > to zero so the list is never traversed, only the first message is
> > > received every time.
> > > 
> > > I am not sure if this is the correct response from netlink, however
> > > the program is stucked here.
> > >
> > > Any ideas?
> > > Please CC me in reply.
> > 
> > That is invalid message to my understanding. Perhaps there's some new
> > extensions that allow it. Upstream (linux kernel) RTA_OK does do
> > additional checks against this situation.
> > 
> > The same issue probably affects if_nameindex.
> > 
> > I think the following should fix it:
> > 
> > diff --git a/src/network/netlink.h b/src/network/netlink.h
> > index 20700ac5..00dc7172 100644
> > --- a/src/network/netlink.h
> > +++ b/src/network/netlink.h
> > @@ -80,13 +80,13 @@ struct ifaddrmsg {
> >  #define NLMSG_DATALEN(nlh)	((nlh)->nlmsg_len-sizeof(struct nlmsghdr))
> >  #define NLMSG_DATAEND(nlh)	((char*)(nlh)+(nlh)->nlmsg_len)
> >  #define NLMSG_NEXT(nlh)		(struct nlmsghdr*)((char*)(nlh)+NETLINK_ALIGN((nlh)->nlmsg_len))
> > -#define NLMSG_OK(nlh,end)	((char*)(end)-(char*)(nlh) >= sizeof(struct nlmsghdr))
> > +#define NLMSG_OK(nlh,end)	((char*)(end)-(char*)(nlh) >= sizeof(struct nlmsghdr) && (nlh)->nlmsg_len >= sizeof(struct nlmsghdr))

Here is an example of the same macros from the kernel sources:

include/uapi/linux/netlink.h:

#define NLMSG_OK(nlh,len) ((len) >= (int)sizeof(struct nlmsghdr) && \
                           (nlh)->nlmsg_len >= sizeof(struct nlmsghdr) && \
                           (nlh)->nlmsg_len <= (len))

It contains one more expression to check that an end of a message is in a buffer.

I think it makes sense to add this expression too.

> >  
> >  #define RTA_DATA(rta)		((void*)((char*)(rta)+sizeof(struct rtattr)))
> >  #define RTA_DATALEN(rta)	((rta)->rta_len-sizeof(struct rtattr))
> >  #define RTA_DATAEND(rta)	((char*)(rta)+(rta)->rta_len)
> >  #define RTA_NEXT(rta)		(struct rtattr*)((char*)(rta)+NETLINK_ALIGN((rta)->rta_len))
> > -#define RTA_OK(nlh,end)		((char*)(end)-(char*)(rta) >= sizeof(struct rtattr))
> > +#define RTA_OK(rta,end)		((char*)(end)-(char*)(rta) >= sizeof(struct rtattr) && (rta)->rta_len >= sizeof(struct rtattr))

#define RTA_OK(rta,len) ((len) >= (int)sizeof(struct rtattr) && \
                         (rta)->rta_len >= sizeof(struct rtattr) && \
                         (rta)->rta_len <= (len))

> >  
> >  #define NLMSG_RTA(nlh,len)	((void*)((char*)(nlh)+sizeof(struct nlmsghdr)+NETLINK_ALIGN(len)))
> >  #define NLMSG_RTAOK(rta,nlh)	RTA_OK(rta,NLMSG_DATAEND(nlh))
> > 
> > 
> > Could you try if this fixes it?
> 
> I'm still waiting to hear whether this fixed it.
> 
> > You will probably need to 'make clean' or at least force recompilation
> > of src/network/{getifaddrs,if_nameindex,netlink}.c as the netlink.h
> > dependency is not picked up by the makefile automatically.
> > 
> > @dalias, if the above looks good to you, I am happy to submit properly
> > formatted git patch for it.
> 
> I don't see anything obvious wrong with the proposed patch, but it
> would be nice to have a better understanding of why it's needed and
> whether this is a workaround for a kernel bug (present in which
> kernels?) or something else.
> 
> Rich