7:31 p.m.
The NLMSG_NEXT macro calculates the next nlmsg and updates the len
field:
#define NLMSG_NEXT(nlh,len) ((len) -= NLMSG_ALIGN((nlh)->nlmsg_len), \
(struct nlmsghdr*)(((char*)(nlh)) + NLMSG_ALIGN((nlh)->nlmsg_len)))
That means nlmsg_len needs to be an multiple of NLMSG_ALIGNTO to avoid
an underflow in len. But there are message which do not have a valid
lenght:
Breakpoint 1, nlmon_receive (io=0x4da210, user_data=0x4d4cc0) at monitor/nlmon.c:6947
6947 printf("malformed packet\n");
(gdb) bt
#0 nlmon_receive (io=0x4da210, user_data=0x4d4cc0) at monitor/nlmon.c:6947
#1 0x0000000000439a91 in io_callback (fd=7, events=1, user_data=0x4da210) at
ell/io.c:126
#2 0x0000000000438861 in l_main_iterate (timeout=-1) at ell/main.c:473
#3 0x0000000000438968 in l_main_run () at ell/main.c:520
#4 0x0000000000438c80 in l_main_run_with_signal (callback=0x4038e6
<signal_handler>, user_data=0x0) at ell/main.c:642
#5 0x0000000000403cc9 in main (argc=3, argv=0x7fffffffec28) at monitor/main.c:806
(gdb) p nlmsg->nlmsg_len
$5 = 17
By adding an lenght check after each processed message garantees that
we do not underflow. The downside is that as soon an invalid length is
spotted the processing stops.
---
monitor/nlmon.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/monitor/nlmon.c b/monitor/nlmon.c
index 6b0afef8a497..5498e9fa0f23 100644
--- a/monitor/nlmon.c
+++ b/monitor/nlmon.c
@@ -6941,6 +6941,11 @@ static bool nlmon_receive(struct l_io *io, void *user_data)
nlmon_message(nlmon, tv, tp, nlmsg);
break;
}
+
+ if (nlmsg->nlmsg_len & (NLMSG_ALIGNTO-1)) {
+ printf("malformed packet\n");
+ break;
+ }
}
return true;
--
2.24.1
1:36 a.m.
Hi Daniel,
On 1/19/20 10:31 AM, Daniel Wagner wrote:
The NLMSG_NEXT macro calculates the next nlmsg and updates the len
field:
#define NLMSG_NEXT(nlh,len) ((len) -= NLMSG_ALIGN((nlh)->nlmsg_len), \
(struct nlmsghdr*)(((char*)(nlh)) + NLMSG_ALIGN((nlh)->nlmsg_len)))
That means nlmsg_len needs to be an multiple of NLMSG_ALIGNTO to avoid
an underflow in len. But there are message which do not have a valid
lenght:
hmm, do you have a pcap of such a message?
Breakpoint 1, nlmon_receive (io=0x4da210, user_data=0x4d4cc0) at
monitor/nlmon.c:6947
6947 printf("malformed packet\n");
(gdb) bt
#0 nlmon_receive (io=0x4da210, user_data=0x4d4cc0) at monitor/nlmon.c:6947
#1 0x0000000000439a91 in io_callback (fd=7, events=1, user_data=0x4da210) at
ell/io.c:126
#2 0x0000000000438861 in l_main_iterate (timeout=-1) at ell/main.c:473
#3 0x0000000000438968 in l_main_run () at ell/main.c:520
#4 0x0000000000438c80 in l_main_run_with_signal (callback=0x4038e6
<signal_handler>, user_data=0x0) at ell/main.c:642
#5 0x0000000000403cc9 in main (argc=3, argv=0x7fffffffec28) at monitor/main.c:806
(gdb) p nlmsg->nlmsg_len
$5 = 17
By adding an lenght check after each processed message garantees that
we do not underflow. The downside is that as soon an invalid length is
spotted the processing stops.
Hmm, isn't NLMSG_OK already enough for this? It checks that length is
positive and above a certain threshold?
---
monitor/nlmon.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/monitor/nlmon.c b/monitor/nlmon.c
index 6b0afef8a497..5498e9fa0f23 100644
--- a/monitor/nlmon.c
+++ b/monitor/nlmon.c
@@ -6941,6 +6941,11 @@ static bool nlmon_receive(struct l_io *io, void *user_data)
nlmon_message(nlmon, tv, tp, nlmsg);
break;
}
+
+ if (nlmsg->nlmsg_len & (NLMSG_ALIGNTO-1)) {
+ printf("malformed packet\n");
+ break;
+ }
Also I'm not sure whether this really holds for messages that are
'last'. There's no sense to check or enforce alignment in such a case/
}
return true;
Regards,
-Denis
12:05 p.m.
On Tue, Jan 21, 2020 at 04:36:37PM -0600, Denis Kenzior wrote:
Hi Daniel,
On 1/19/20 10:31 AM, Daniel Wagner wrote:
> The NLMSG_NEXT macro calculates the next nlmsg and updates the len
> field:
>
> #define NLMSG_NEXT(nlh,len) ((len) -= NLMSG_ALIGN((nlh)->nlmsg_len), \
> (struct nlmsghdr*)(((char*)(nlh)) + NLMSG_ALIGN((nlh)->nlmsg_len)))
>
> That means nlmsg_len needs to be an multiple of NLMSG_ALIGNTO to avoid
> an underflow in len. But there are message which do not have a valid
> lenght:
>
hmm, do you have a pcap of such a message?
I can try to capture one. The problem is that the monitor crashes
without this patch.
> Breakpoint 1, nlmon_receive (io=0x4da210, user_data=0x4d4cc0)
at monitor/nlmon.c:6947
> 6947 printf("malformed packet\n");
> (gdb) bt
> #0 nlmon_receive (io=0x4da210, user_data=0x4d4cc0) at monitor/nlmon.c:6947
> #1 0x0000000000439a91 in io_callback (fd=7, events=1, user_data=0x4da210) at
ell/io.c:126
> #2 0x0000000000438861 in l_main_iterate (timeout=-1) at ell/main.c:473
> #3 0x0000000000438968 in l_main_run () at ell/main.c:520
> #4 0x0000000000438c80 in l_main_run_with_signal (callback=0x4038e6
<signal_handler>, user_data=0x0) at ell/main.c:642
> #5 0x0000000000403cc9 in main (argc=3, argv=0x7fffffffec28) at
monitor/main.c:806
> (gdb) p nlmsg->nlmsg_len
> $5 = 17
>
> By adding an lenght check after each processed message garantees that
> we do not underflow. The downside is that as soon an invalid length is
> spotted the processing stops.
Hmm, isn't NLMSG_OK already enough for this? It checks that length is
positive and above a certain threshold?
No, the problem is how the len is calculated. In the above example len
will be -3
len = 17 - 20
and from there the next pointer nlmsg pointer will be calculated,
which is invalid.
> ---
> monitor/nlmon.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/monitor/nlmon.c b/monitor/nlmon.c
> index 6b0afef8a497..5498e9fa0f23 100644
> --- a/monitor/nlmon.c
> +++ b/monitor/nlmon.c
> @@ -6941,6 +6941,11 @@ static bool nlmon_receive(struct l_io *io, void *user_data)
> nlmon_message(nlmon, tv, tp, nlmsg);
> break;
> }
> +
> + if (nlmsg->nlmsg_len & (NLMSG_ALIGNTO-1)) {
> + printf("malformed packet\n");
> + break;
> + }
Also I'm not sure whether this really holds for messages that are 'last'.
There's no sense to check or enforce alignment in such a case/
In my example it is the last message. I've printed out the nlmsg_len
and all of them seem to be a mulitple of NLMSG_ALIGNTO.
Thanks,
Daniel
7:21 p.m.
Hi Daniel,
On 1/22/20 3:05 AM, Daniel Wagner wrote:
On Tue, Jan 21, 2020 at 04:36:37PM -0600, Denis Kenzior wrote:
> Hi Daniel,
>
> On 1/19/20 10:31 AM, Daniel Wagner wrote:
>> The NLMSG_NEXT macro calculates the next nlmsg and updates the len
>> field:
>>
>> #define NLMSG_NEXT(nlh,len) ((len) -= NLMSG_ALIGN((nlh)->nlmsg_len), \
>> (struct nlmsghdr*)(((char*)(nlh)) + NLMSG_ALIGN((nlh)->nlmsg_len)))
>>
>> That means nlmsg_len needs to be an multiple of NLMSG_ALIGNTO to avoid
>> an underflow in len. But there are message which do not have a valid
>> lenght:
>>
>
> hmm, do you have a pcap of such a message?
I can try to capture one. The problem is that the monitor crashes
without this patch.
Are you running the latest version btw? From your backtrace it seems
like the nlmon.c file I have is different from yours.
>> Breakpoint 1, nlmon_receive (io=0x4da210, user_data=0x4d4cc0) at
monitor/nlmon.c:6947
>> 6947 printf("malformed packet\n");
>> (gdb) bt
>> #0 nlmon_receive (io=0x4da210, user_data=0x4d4cc0) at monitor/nlmon.c:6947
>> #1 0x0000000000439a91 in io_callback (fd=7, events=1, user_data=0x4da210) at
ell/io.c:126
>> #2 0x0000000000438861 in l_main_iterate (timeout=-1) at ell/main.c:473
>> #3 0x0000000000438968 in l_main_run () at ell/main.c:520
>> #4 0x0000000000438c80 in l_main_run_with_signal (callback=0x4038e6
<signal_handler>, user_data=0x0) at ell/main.c:642
>> #5 0x0000000000403cc9 in main (argc=3, argv=0x7fffffffec28) at
monitor/main.c:806
>> (gdb) p nlmsg->nlmsg_len
>> $5 = 17
>>
>> By adding an lenght check after each processed message garantees that
>> we do not underflow. The downside is that as soon an invalid length is
>> spotted the processing stops.
>
> Hmm, isn't NLMSG_OK already enough for this? It checks that length is
> positive and above a certain threshold?
No, the problem is how the len is calculated. In the above example len
will be -3
len = 17 - 20
Right, I get that. But I think the issue isn't what you think it is
exactly. Here's the rough loop inside nlmon_receive:
size_t nlmsg_len;
nlmsg_len = bytes_read;
for (nlmsg = foo; NLMSG_OK(nlmsg, nlmsg_len;
nlmsg = NLMSG_NEXT(nlmsg, nlmsg_len)) {
// process message nlmsg
}
NLMSG_OK and NLMSG_NEXT are macros in linux/netlink.h. The problem is
that NLMSG_OK and NLMSG_NEXT expect to operate on an int according to
'man netlink', but they don't actually cast the argument to an int.
So what happens is that nlmsg_len underflows, wraps around to some large
number and the crash happens.
See if changing the nlmsg_len type to an int fixes this problem.
<snip>
>
> Also I'm not sure whether this really holds for messages that are
'last'.
> There's no sense to check or enforce alignment in such a case/
In my example it is the last message. I've printed out the nlmsg_len
and all of them seem to be a mulitple of NLMSG_ALIGNTO.
Then I don't see how a crash would happen, wouldn't nlmsg_len just
become 0 then?
Regards,
-Denis
8:26 p.m.
Hi Denis,
On Wed, Jan 22, 2020 at 10:21:10AM -0600, Denis Kenzior wrote:
On 1/22/20 3:05 AM, Daniel Wagner wrote:
> On Tue, Jan 21, 2020 at 04:36:37PM -0600, Denis Kenzior wrote:
> > Hi Daniel,
> >
> > On 1/19/20 10:31 AM, Daniel Wagner wrote:
> > > The NLMSG_NEXT macro calculates the next nlmsg and updates the len
> > > field:
> > >
> > > #define NLMSG_NEXT(nlh,len) ((len) -=
NLMSG_ALIGN((nlh)->nlmsg_len), \
> > > (struct nlmsghdr*)(((char*)(nlh)) +
NLMSG_ALIGN((nlh)->nlmsg_len)))
> > >
> > > That means nlmsg_len needs to be an multiple of NLMSG_ALIGNTO to avoid
> > > an underflow in len. But there are message which do not have a valid
> > > lenght:
> > >
> >
> > hmm, do you have a pcap of such a message?
>
> I can try to capture one. The problem is that the monitor crashes
> without this patch.
Are you running the latest version btw? From your backtrace it seems like
the nlmon.c file I have is different from yours.
Oh, I thought I did the patch against head, but well. It was against:
2a7c4a9c3d22 ("doc: Fix wrong interface name")
Right, I get that. But I think the issue isn't what you think it
is
exactly. Here's the rough loop inside nlmon_receive:
size_t nlmsg_len;
nlmsg_len = bytes_read;
for (nlmsg = foo; NLMSG_OK(nlmsg, nlmsg_len;
nlmsg = NLMSG_NEXT(nlmsg, nlmsg_len)) {
// process message nlmsg
}
NLMSG_OK and NLMSG_NEXT are macros in linux/netlink.h. The problem is that
NLMSG_OK and NLMSG_NEXT expect to operate on an int according to 'man
netlink', but they don't actually cast the argument to an int.
So what happens is that nlmsg_len underflows, wraps around to some large
number and the crash happens.
It's not the current message which triggers the crash, it's the next
loop iteration which triggers the crash.
See if changing the nlmsg_len type to an int fixes this problem.
Indeed, changing the type fixed it:
$ git diff
diff --git a/monitor/nlmon.c b/monitor/nlmon.c
index 77f5dda42946..45bd37202f9d 100644
--- a/monitor/nlmon.c
+++ b/monitor/nlmon.c
@@ -6876,7 +6876,7 @@ static bool nlmon_receive(struct l_io *io, void *user_data)
unsigned char buf[8192];
unsigned char control[32];
ssize_t bytes_read;
- size_t nlmsg_len;
+ ssize_t nlmsg_len;
int fd;
fd = l_io_get_fd(io);
> > Also I'm not sure whether this really holds for
messages that are 'last'.
> > There's no sense to check or enforce alignment in such a case/
>
> In my example it is the last message. I've printed out the nlmsg_len
> and all of them seem to be a mulitple of NLMSG_ALIGNTO.
>
Then I don't see how a crash would happen, wouldn't nlmsg_len just become 0
then?
The check would just abort the loop as soon a the lenght check
triggers. I've tested the new version and nlmsg_len still got
underflow but the macro aborts the loop now.
Do you want me to send a new patch or are you going to fix it
yourself? I don't mind :)
Thanks,
Daniel
8:53 p.m.
Hi Daniel,
> NLMSG_OK and NLMSG_NEXT are macros in linux/netlink.h. The
problem is that
> NLMSG_OK and NLMSG_NEXT expect to operate on an int according to 'man
> netlink', but they don't actually cast the argument to an int.
>
> So what happens is that nlmsg_len underflows, wraps around to some large
> number and the crash happens.
It's not the current message which triggers the crash, it's the next
loop iteration which triggers the crash.
Right. So after the last valid message, NLMSG_NEXT gets called which
causes the underflow. But since the type is unsigned, nlmsg_len just
becomes a large number. Then NLMSG_OK is invoked and thinks nlmsg_len
is still all okay.
>
> Then I don't see how a crash would happen, wouldn't nlmsg_len just become 0
> then?
The check would just abort the loop as soon a the lenght check
triggers. I've tested the new version and nlmsg_len still got
underflow but the macro aborts the loop now.
Right, that's the intent of NLMSG_OK and why it uses ints.
I still think the last message isn't *actually* aligned, but without
crash data I can't prove it :) It may be that most messages are indeed
aligned, but the ones you're seeing this on are somehow special.
Otherwise I don't see how we haven't detected this crash for so long.
Do you want me to send a new patch or are you going to fix it
yourself? I don't mind :)
I went ahead and pushed 8b489d5df283 ("monitor: Fix crash") with your
name on the Reported-By.
Regards,
-Denis
9:41 p.m.
On Wed, Jan 22, 2020 at 11:53:47AM -0600, Denis Kenzior wrote:
I still think the last message isn't *actually* aligned, but
without crash
data I can't prove it :) It may be that most messages are indeed aligned,
but the ones you're seeing this on are somehow special. Otherwise I don't
see how we haven't detected this crash for so long.
Let me help out with that :)
(gdb) bt full
#0 0x000000000041ba55 in nlmon_receive (io=0x4da210, user_data=0x4d4cc0) at
monitor/nlmon.c:6930
nlmon = 0x4d4cc0
nlmsg = 0x800000ffd2f4
msg = {msg_name = 0x7fffffffe920, msg_namelen = 20, msg_iov = 0x7fffffffe910,
msg_iovlen = 1,
msg_control = 0x7fffffffc8c0, msg_controllen = 32, msg_flags = 0}
sll = {sll_family = 17, sll_protocol = 0, sll_ifindex = 6, sll_hatype = 824,
sll_pkttype = 4 '\004',
sll_halen = 0 '\000', sll_addr =
"\000\000\000\000\000\000\000"}
iov = {iov_base = 0x7fffffffc8e0, iov_len = 8192}
cmsg = 0x0
copy_tv = {tv_sec = 1579718221, tv_usec = 657974}
copy_tp = {tp_status = 0, tp_len = 0, tp_snaplen = 0, tp_mac = 0, tp_net = 0,
tp_vlan_tci = 0,
tp_vlan_tpid = 0}
tv = 0x7fffffffe900
tp = 0x0
proto_type = 0
buf =
"\021\000\000\000\026\000\001\003\000\000\000\000wU\000\000\000\200\200\000\376\t\000\001\000\000\000\000\b\000\017\000\376\000\000\000\024\000\001\000*\002&\360\000@\000\000\000\000\000\000\027\333[\033\024\000\002",
'\000' <repeats 17 times>,
"\024\000\a\000\376\200\000\000\000\000\000\000\342\325^\377\376\343\036\024\f\000\b\000\b\000\n\000@\000\000\000\b\000\006\000\000\004\000\000\024\000\005\000\376\200\000\000\000\000\000\000\362\237\302\377\376¬\337\b\000\004\000\002\000\000\000$\000\f\000\002\000\000\000\000\316\065",
'\000' <repeats 25 times>,
"\005\000\024\000\001\000\000\000\024\000\006\000\377\377\377\377\377\377\377\377\375"...
control = "
\000\000\000\000\000\000\000\001\000\000\000\035\000\000\000M\226(^\000\000\000\000\066\n\n\000\000\000\000"
bytes_read = 17
nlmsg_len = 18446744073692771837
fd = 7
(gdb) p iov.iov_base
$1 = (void *) 0x7fffffffc8e0
(gdb) p *(struct nlmsghdr *)iov.iov_base
$2 = {nlmsg_len = 17, nlmsg_type = 22, nlmsg_flags = 769, nlmsg_seq = 0, nlmsg_pid =
21879}
> Do you want me to send a new patch or are you going to fix it
> yourself? I don't mind :)
I went ahead and pushed 8b489d5df283 ("monitor: Fix crash") with your name
on the Reported-By.
Thanks!
Daniel
472
days inactive
475
days old
6 comments
2 participants
participants (2)
-
Daniel Wagner -
Denis Kenzior