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 have a regression test for this based on Alexander Markert code + rework by mpi@, do you want me to commit it right now ?