Re: [PATCH] monitor: Add message length check to nlmon_receice

[Denis Kenzior <denkenz@gmail.com>]

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

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