Bug 16919 - recvmsg standard compliance
Summary: recvmsg standard compliance
Status: WAITING
Alias: None
Product: glibc
Classification: Unclassified
Component: network (show other bugs)
Version: unspecified
: P2 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
: 18233 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-05-07 16:26 UTC by Lana Black
Modified: 2018-10-24 16:29 UTC (History)
6 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:
fweimer: security-


Attachments
Patch to fix cmsghdr member type (475 bytes, patch)
2014-05-07 16:27 UTC, Lana Black
Details | Diff
Patch to fix struct msghdr members types (482 bytes, patch)
2014-05-07 16:27 UTC, Lana Black
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Lana Black 2014-05-07 16:26:31 UTC
Types of several members of both struct cmsghdr and struct msghdr are different from those specified in the standard.

The patches attached below fix this issue without breaking the compatibility with the linux kernel.
Comment 1 Lana Black 2014-05-07 16:27:15 UTC
Created attachment 7591 [details]
Patch to fix cmsghdr member type
Comment 2 Lana Black 2014-05-07 16:27:52 UTC
Created attachment 7592 [details]
Patch to fix struct msghdr members types
Comment 3 joseph@codesourcery.com 2014-05-07 17:08:02 UTC
Padding should use __glibc_reserved* names.

To make part of an existing field into padding like this, the following 
need to be true:

* If the kernel reads the field, all existing kernel versions supported by 
glibc already treat that part of the field as padding, so they will not 
misinterpret uninitialized data there.  If that's not the case, glibc 
needs to initialize the padding appropriately before passing the structure 
to the kernel.

* If the kernel writes the field, it gives an overflow error rather than 
treating that part of the field as something to which a significant value 
may be written.  If it treats it as significant, glibc needs to check in 
userspace for overflow and set an error in that case (just as the 
asm-generic implementations of 32-bit stat functions do).

* If user programs write the field, so that for old programs the high bits 
are significant but for new programs the high bits are padding, symbol 
versioning needs to be used to avoid misinterpreting values written there 
by existing binaries.

* If user programs read the field, so that old programs may interpret high 
bits written by glibc or the kernel as significant, those bits need 
initializing appropriately for the existing programs.
Comment 4 Lana Black 2014-05-07 22:21:22 UTC
(In reply to joseph@codesourcery.com from comment #3)
> Padding should use __glibc_reserved* names.
> 
> To make part of an existing field into padding like this, the following 
> need to be true:
> 
> * If the kernel reads the field, all existing kernel versions supported by 
> glibc already treat that part of the field as padding, so they will not 
> misinterpret uninitialized data there.  If that's not the case, glibc 
> needs to initialize the padding appropriately before passing the structure 
> to the kernel.
> 
> * If the kernel writes the field, it gives an overflow error rather than 
> treating that part of the field as something to which a significant value 
> may be written.  If it treats it as significant, glibc needs to check in 
> userspace for overflow and set an error in that case (just as the 
> asm-generic implementations of 32-bit stat functions do).
> 
> * If user programs write the field, so that for old programs the high bits 
> are significant but for new programs the high bits are padding, symbol 
> versioning needs to be used to avoid misinterpreting values written there 
> by existing binaries.
> 
> * If user programs read the field, so that old programs may interpret high 
> bits written by glibc or the kernel as significant, those bits need 
> initializing appropriately for the existing programs.

Thanks for the tip. I should create appropriate __libc_{sendmsg,recvmsg} functions for that, am I right?
Comment 5 joseph@codesourcery.com 2014-05-07 23:23:58 UTC
On Wed, 7 May 2014, sickmind at i2pmail dot org wrote:

> Thanks for the tip. I should create appropriate __libc_{sendmsg,recvmsg}
> functions for that, am I right?

Exactly what's appropriate depends on the details of the interfaces - what 
libc interfaces use these structures as inputs, outputs or both, and what 
kernel interfaces libc uses with them (and again, whether as inputs, 
outputs or both), and how the kernel handles the high bits in question.

The patch submission (to libc-alpha, see the contribution checklist on the 
wiki) will need to contain a detailed analysis of those issues that 
justifies why the changes in the patch don't cause any problems for 
existing binaries or cause the kernel interface to be used incorrectly.  
If you write up that analysis before implementing the patch, it may help 
people advise on what should or should not be in the patch.
Comment 6 Rich Felker 2014-07-20 16:32:31 UTC
Fixing this issue is a lot harder than it looks, especially since the kernel folks went and added sendmmsg/recvmmsg that also use these structures. For both recvmmsg and sendmmsg the msghdr structures lie in memory that's (by the interface contract) writable by the function so it's easy to just patch zeros into the padding slots before making the syscall. However sendmmsg's msghdr points to an already-filled control buffer containing cmsghdr structures that may contain junk in their padding areas, and this buffer is NOT writable. So you have to do something like copying the control buffers (which may be of very large total length) to a copy, modifying the msghdr structures to point to the copies, making the syscall, then putting it all back. At that point it might be easier (this is what we do in musl) to just loop calling sendmsg rather than bothering with the sendmmsg syscall...

We should really just demand that this be fixed on the kernel side, so that when glibc drops support for old kernels somewhere down the line, these ugly hacks can be dropped and the syscalls can just be safely usable directly.
Comment 7 Lana Black 2014-07-26 13:38:45 UTC
(In reply to Rich Felker from comment #6)
> We should really just demand that this be fixed on the kernel side, so that
> when glibc drops support for old kernels somewhere down the line, these ugly
> hacks can be dropped and the syscalls can just be safely usable directly.

What about the usual kernel devs' motto about not breaking the userspace?
Comment 8 Rich Felker 2014-07-26 18:13:00 UTC
I don't think fixing the kernel to ignore the upper 32 bits of these wrongly-64-bit fields would "break userspace". It would mask bugs in old applications that wrote huge (>0xffffffff) values to the fields and expected to get an error from doing so rather than for the high bits to be ignored, but I really doubt any such applications exist. For all applications using the interfaces correctly, old apps with the wrong struct definition would already be storing zeros in the high bits, and new applications with the right struct definition would be leaving junk in the upper bits for the kernel (or, for the time being, glibc when it does the fixup in userspace to support old kernels) to ignore.
Comment 9 Joseph Myers 2015-04-10 17:36:12 UTC
*** Bug 18233 has been marked as a duplicate of this bug. ***
Comment 10 Adhemerval Zanella 2016-06-10 15:04:04 UTC
After some discussion in libc-alpha about this POSIX compliance fix, I see that GLIBC should indeed revert back to previous definition of msghdr and cmsghdr and implementation of sendmsg, recvmsg, sendmmsg, recvmmsg due some reasons:

 * The possible issue where the syscalls wrapper add the compatibility
   layer is quite limited in scope and range.  And kernel current
   also add some limits to the values on the internal msghdr and
   cmsghdr fields:
    
   - msghdr::msg_iovlen larger than UIO_MAXIOV (1024) returns EMSGSIZE.
   - msghdr::msg_controllen larger than INT_MAX returns ENOBUFS.
    
 * There is a small performance hit for recvmsg/sendmsg/recmmsg which
   is neglectable, but it is a big hit for sendmmsg since now instead
   of calling the syscall for the packed structure, GLIBC is calling
   multiple sendmsg.  This defeat the very existence of the syscall.
    
 * It currently breaks libsanitizer build on GCC [1] (I fixed on compiler-rt).
   However the fix is incomplete because it does add any runtime check
   since libsanitizer currently does not have any facility to intercept
   symbols with multiple version [2].
    
   This, along with incorret dlsym/dlvsym return for versioned symbol due
   another bug [3], makes hard to interpose versioned symbols.
    
Also, current approach of fixing GCC PR#71445 leads to half-baked solutions without versioned symbol interposing.
    
This patch basically reverts commits 2f0dc39029ae08, 222c2d7f4357d66, af7f7c7ec8dea1.  I decided to not revert abf29edd4a3918 (Adjust kernel-features.h defaults for recvmsg and sendmsg) mainly because it does not really address the POSIX compliance original issue and also adds some cleanups.

This is not a state to mark this bug a WONTFIX.
Comment 11 Florian Weimer 2018-10-24 11:35:12 UTC
Do we still plan to fix this bug?  I'm inclined to close it as WONTFIX.
Comment 12 Adhemerval Zanella 2018-10-24 12:03:27 UTC
(In reply to Florian Weimer from comment #11)
> Do we still plan to fix this bug?  I'm inclined to close it as WONTFIX.

The fix it self it just a matter to adapt 2f0dc39029ae08, 222c2d7f4357d66, af7f7c7ec8dea1 to master. However to proper fix I think we should first revise and fix dlsym/dlvsym issues (BZ#1319, BZ#12977, BZ#14932) so libraries that override libc symbols (like sanitizers) can access correctly all the versions.

In any case, I think the overall gain on fix it is quite small and I think it would be ok to just close it.
Comment 13 Rich Felker 2018-10-24 14:38:17 UTC
It would be really disappointing to see this closed. glibc has made a lot of progress on not treating this kind of issue as a joke and I feel like this would be a setback.
Comment 14 Florian Weimer 2018-10-24 14:42:52 UTC
(In reply to Rich Felker from comment #13)
> It would be really disappointing to see this closed. glibc has made a lot of
> progress on not treating this kind of issue as a joke and I feel like this
> would be a setback.

It's a kernel bug.  I consider rewriting the ancillary data too risky.  The kernel ABI around recvmsg/sendmsg isn't totally stable, too.
Comment 15 Rich Felker 2018-10-24 15:16:50 UTC
It is a kernel bug, but there are A LOT of kernel bugs that are conformance bugs where userspace (libc) has to patch around the kernel behavior to get a conforming environment. Things like wrong errno values, spurious failure in a corner case that should be success or vice versa, etc.

Maybe this is getting off-topic for an individual bug, but "WONTFIX" has a really bad reputation and implication of finality. If it's really too risky to fix now or in the forseeable future, it would be nice to have a state expressing that an issue is blocked (has no current plausible path forward) or deferred without an explicit and willful "intent not to fix".
Comment 16 joseph@codesourcery.com 2018-10-24 16:29:30 UTC
On Wed, 24 Oct 2018, bugdal at aerifal dot cx wrote:

> Maybe this is getting off-topic for an individual bug, but "WONTFIX" has a
> really bad reputation and implication of finality. If it's really too risky to
> fix now or in the forseeable future, it would be nice to have a state
> expressing that an issue is blocked (has no current plausible path forward) or
> deferred without an explicit and willful "intent not to fix".

I agree WONTFIX should be avoided except for bugs in removed functionality 
(e.g. we remove a port which has some open port-specific bugs).  If 
something depends on an external fix (kernel, compiler, standard DR 
resolution, etc.), SUSPENDED is the appropriate state (WAITING is 
different, it means waiting for more information from the submitter).  If 
we reach consensus that something is not a bug and the change requested by 
the submitter is not desirable, INVALID should be used instead of WONTFIX.