This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] network: Fix cmsghdr padding in sendmsg (BZ#16919)



On 26/05/2016 18:07, Zack Weinberg wrote:
> Please forgive terseness, I'm writing on a phone.
> 
> On May 26, 2016 8:46 PM, "Adhemerval Zanella" <adhemerval.zanella@linaro.org <mailto:adhemerval.zanella@linaro.org>> wrote:
>> On 26/05/2016 16:25, Zack Weinberg wrote:
>> > On May 26, 2016 3:37 PM, "Adhemerval Zanella" <adhemerval.zanella@linaro.org <mailto:adhemerval.zanella@linaro.org>> wrote:
>> >>
>> >> This patch fixes the remaining issue in sendmsg POSIX compliance by
>> >> adjusting the cmsghdr padding accordingly for 64-bits ABIs.  Since
>> >> function contract does not allow to modify it in place, a temporary
>> >> buffer instead.
>> >
>> > I don't think this can possibly be safe. It's not just a matter of the size limit; if there's *any way at all* for the caller to observe that a copy occurred -- that the pointer it supplied was not handed directly to the kernel -- including obscure cmsg types -- this will break some existing application.
>> >
>>
>> I do not think this might be an issue since it is transparent to the
>> application.
> 
> You haven't proven that to my satisfaction. It's not just about (c)msghdr pointing to read-only data
> It's a question of whether there is now, or could conceivably be in the future, *any* way for caller to observe a copy. Since this interface is extensible by (at least) adding cmsg opcodes, that probably isn't a bar you can ever clear.

It does not matter if the caller see or not a copy in sendmsg, what matter
is if the syscall contract is respected.  And afaik cmsghdr defined in a way
where is ancillary is allocated using a flexibe array with its length defined
by cmsg_len, so all the data that requires to be passed on kernel is contained
in cmsg_hdr defined memory extension.

So my understanding is the cmsghdr must be well formed where kernel can walk
using the defined CMSG_* macros.


>> It needs to be cleared because POSIX states the cmsg_len to be a socklen_t
>> size, which for 64-bits architecture on Linux is still 32-bit unsigned
>> types.  So to be portable, programs can not rely on passing cmsghdr
>> larger than socketlen_t size.
> 
> This doesn't answer my question, but I think what you're saying is that the kernel defines cmsg_len as size_t even though POSIX specifies socklen_t, so you can have the application fill in only the low 32 bits but the kernel reads 64. Thing is, glibc shouldn't be trying to paper that over. It is more important for the C library to faithfully reflect what the kernel interface really is, than for it to check this particular conformance box.
> 

GLIBC general position is to follow POSIX standard to provide a portable
interface and I do not see a compelling reason to not do so in this
particular case.  Limiting cmsg_len to 32-bits is still a quite large 
buffer limit size and I doubt very much if any application is or will 
exceed this size anytime soon.

Also, limiting the size does not compromise the interface since program
may issue two or more syscalls if the used cmsghdr buffer does not fit
in a socklen_t (as for every other socket call).

> zw
> 


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]