Bug 166248

Created attachment 117865 [details]
Patch and comments from Al Viro

Notified security, vendor-sec. 
Embargo set for one week, 20050907:12

The RHEL4 patch appears to cause portmap to fail on ppc64. 

recvmsg(3, {msg_name(16)={sa_family=AF_INET, sin_port=htons(963),
sin_addr=inet_addr("127.0.0.1")},
msg_iov(1)=[{"s\37\273a\0\0\0\0\0\0\0\2\0\1\206\240\0\0\0\2\0\0\0\1\0"...,
8800}], msg_controllen=24, {cmsg_len=24, cmsg_level=SOL_IP, cmsg_type=, ...},
msg_flags=0x80000000}, 0) = 56
sendmsg(3, {msg_name(16)={sa_family=AF_INET, sin_port=htons(963),
sin_addr=inet_addr("127.0.0.1")},
msg_iov(1)=[{"s\37\273a\0\0\0\1\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 28}],
msg_controllen=24, {cmsg_len=24, cmsg_level=SOL_IP, cmsg_type=, ...},
msg_flags=0x80000000}, 0) = -1 EINVAL (Invalid argument)

 		tmp = ((ucmlen - CMSG_COMPAT_ALIGN(sizeof(*ucmsg))) +
 		       CMSG_ALIGN(sizeof(struct cmsghdr)));
+		if ((char *)kcmsg_base + kcmlen - (char *)kcmsg < CMSG_ALIGN(tmp))
+			goto Einval;

This is what's causing portmap to die. The test fails:

    c00000001e32bc40 + 28 - c00000001e32bc40 < 32
    tmp is 28 (24-12 + 16)

Should we really be comparing against CMSG_ALIGN(tmp) there instead of just
(tmp)? Or is it the initial calculation loop that's wrong, and should be
rounding up with CMSG_ALIGN() each time it does 'kcmlen += tmp'?

S'far too late on a Friday night to be working this out :)

Created attachment 118407 [details]
Updated patch, correcting CMSG_LEN() usage.

ITYM "didn't add missing CMSG_ALIGN() in the first loop".  ACK.

What happens here is that we had another bug in the same function -
size of kernel buffer had been underestimated in the first loop
since we didn't adjust for alignment.  Another source of overflow,
that is.  Original patch didn't fix that bug, but with proper checks
for overflow in place in the second loop we started to see -EINVAL
instead of silently overflowing the damn thing.

Adding CMSG_ALIGN() in the first loop closes that one for good.  And

grrr....  And we need it both in 2.4 and 2.6.  One note: I would
assign ->cmsg_len the original value of tmp, not the aligned-up
one.  _Probably_ OK either way for all existing uses, but we are
better off with length of payload remaining the same.  So in the
second loop do comparison with CMSG_ALIGN(tmp), then assign
tmp to ->cmsg_len, then align tmp up.

I think cmsg_len should be the aligned one, because that is what the
application would have padded it out to were it native.

ACK, then

Actually, Al, I think I'm wrong.  We should set the cmsg_len to the
pre-aligned calculation.  I am pretty sure this is why David W. sees
that filure in sys_sendmsg().

Created attachment 118435 [details]
Yeah, this one appears to work. Thanks.

Created attachment 118437 [details]
Incremental (for easier DaveM review) patch to do same for RHEL3.

Looks great David, I'll take the RHEL3 side from here.

Created attachment 118445 [details]
Test case for sendmsg() CMSG usage.

This is a test program which specifically tests the code path
being modified by this bug fix.  Since this code path is used only
for 32-bit applications running under a 64-bit kernel, make sure
that the test program is compiled into a 32-bit binary for proper
testing.

Test case passes on RHEL3 and RHEL4 ppc64 with the currently proposed patches.

sending updated patches to security and vendor-sec

Sigh...  One more turd: in the second loop we must do the following:
1) check that we won't overflow
2) check that CMSG_ALIGN(tmp) >= CMSG_ALIGN(sizeof cmsg_header))
3) _then_ set ->cmsg_len
4) then align tmp up.

Otherwise we can still be tricked into overflow - one last header
can be written past the end of buffer, if kcmsg is at the end
of buffer (or just before it) and tmp is small enough (less than
sizeof).

Actually, it's not quite that bad (due to previous checks we know
that tmp will be large enough).  However, we really want to check
for overflow before setting ->cmsg_len - otherwise we can get
one word written just past the end of buffer here.

Created attachment 118475 [details]
incremental to net/compat.c

Created attachment 118486 [details]
Final (hopefully) patch resulting from comment #32

Removing embargo, public today.

An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on the solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHSA-2005-514.html