On Thu, 12 Jul 2018 19:54:26 +0200
Alexander Bluhm <[email protected]> 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 ?
