On Mon, Jul 16 2018, Vincent Gross <vgr...@openbsd.org> wrote:
> On Thu, 12 Jul 2018 19:54:26 +0200
> Alexander Bluhm <alexander.bl...@gmx.net> wrote:
>
>>
>> If it is a temporary problem, that will go away when the content
>> of the socket buffer is sent away, we should block or return
>> EWOULDBLOCK. For a permanent problem return EMSGSIZE. Non atomic
>> operations can be split in smaller chunks, so there are no permanent
>> problems. Control messages are considerd atomic. AF_UNIX needs
>> special treatment as file descriptor passing is dificult. On top
>> of that consider integer overflow.
>>
>> revision 1.100
>> date: 2012/04/24 16:35:08; author: deraadt; state: Exp; lines:
>> +13 -3; In sosend() for AF_UNIX control message sending, correctly
>> calculate the size (internalized ones can be larger on some
>> architectures) for fitting into the socket. Avoid getting confused
>> by sb_hiwat as well. This fixes a variety of issues where sendmsg()
>> would fail to deliver a fd set or fail to wait; even leading to file
>> leakage. Worked on this with claudio for about a week...
>>
>> revision 1.145
>> date: 2016/01/06 10:06:50; author: stefan; state: Exp; lines:
>> +27 -30; Prevent integer overflows in sosend() and soreceive() by
>> converting min()+uiomovei() to ulmin()+uiomove() and re-arranging
>> space computations in sosend(). The soreceive() part was also
>> reported by Martin Natano. ok bluhm@ and also discussed with tedu@
>>
>> So first of all we should split the AF_UNIX cases to keep it readable.
>> And I don't want to change the AF_UNIX code as the commit message
>> indicates that it was hard to develop the current solution.
>>
>> From the bug reports it seems that we should check that the UDP
>> packets and the IP_SENDSRCADDR fit into the socket buffer. If not
>> it is a permanent EMSGSIZE error. So make sure that resid + clen
>> <= so->so_snd.sb_hiwat. Then write it the other way around to
>> prevent signed integer overflow.
>>
>> The result of this considerations is the diff below. I have not
>> tested it. Does the orignal bug go away with it? Some hackathons
>> ago jeremy@ mentioned that the ruby test suite found a bug in this
>> area. So maybe we should try it.
>>
>> Does this make sense?
>>
>> bluhm
>>
>> Index: kern/uipc_socket.c
>> ===================================================================
>> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_socket.c,v
>> retrieving revision 1.225
>> diff -u -p -r1.225 uipc_socket.c
>> --- kern/uipc_socket.c 5 Jul 2018 14:45:07 -0000 1.225
>> +++ kern/uipc_socket.c 12 Jul 2018 17:24:28 -0000
>> @@ -462,10 +462,14 @@ restart:
>> space = sbspace(so, &so->so_snd);
>> if (flags & MSG_OOB)
>> space += 1024;
>> - if ((atomic && resid > so->so_snd.sb_hiwat) ||
>> - (so->so_proto->pr_domain->dom_family != AF_UNIX
>> &&
>> - clen > so->so_snd.sb_hiwat))
>> - snderr(EMSGSIZE);
>> + if (so->so_proto->pr_domain->dom_family == AF_UNIX) {
>> + if (atomic && resid > so->so_snd.sb_hiwat)
>> + snderr(EMSGSIZE);
>> + } else {
>> + if (clen > so->so_snd.sb_hiwat ||
>> + (atomic && resid > so->so_snd.sb_hiwat -
>> clen))
>> + snderr(EMSGSIZE);
>> + }
>> if (space < clen ||
>> (space - clen < resid &&
>> (atomic || space < so->so_snd.sb_lowat))) {
>
> It is indeed much easier to parse. Kudos on spotting the potential
> overflow. Ok vgross@
I like it too, sadly I haven't been able to do extensive testing yet.
And my main laptop's screen died yesterday, so fixing it is the next
thing on my plate...
> I have a regression test for this based on Alexander Markert code +
> rework by mpi@, do you want me to commit it right now ?
I'd say go for it, even if you don't hook it up right now. It's easier
for people to extend the tests it performs if they can just share and
commit diffs.
Thanks,
--
jca | PGP : 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
