On Thu, Jul 12, 2018 at 06:05:14PM +0200, Jeremie Courreges-Anglas wrote:
> --8<--
> 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 (space < clen ||
> (space - clen < resid &&
> (atomic || space < so->so_snd.sb_lowat))) {
> if ((so->so_state & SS_NBIO) || (flags & MSG_DONTWAIT))
> snderr(EWOULDBLOCK);
> sbunlock(so, &so->so_snd);
> error = sbwait(so, &so->so_snd);
> so->so_state &= ~SS_ISSENDING;
> if (error)
> goto out;
> goto restart;
> }
> space -= clen;
> -->8--
>
> I hate those conditions. I do not fully understand their true intent
> and I feel they are already brittle; I'm afraid that adding more
> conditionals will worsen clarity. How are those three (actually way
> more than three) conditions actually supposed to interact?
>
> If people have a clearer idea than I do about what exactly should happen
> here, please share.
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))) {
