Re: [PATCH 2/5] __FD_ELT: Implement correct buffer overflow check

[KOSAKI Motohiro <kosaki dot motohiro at gmail dot com>]

>> Does compiling ruby (or similar code) with this header
>> result in calls to __fdelt_buffer_warn or __fdelt_buffer_chk?
> 
> Unfortunately, No. __builtin_object_size() require compiler know the
> buffer size.
> In the other words, it doesn't work if an allocate function and
> FD_{SET,CLR} functions
> doesn't exist in the same place. This is the same limitation with
> other string buffer
> overflow checks.

I inspected several other project codes.

sudo
------------------------
check_input(int ttyfd, double *speed)
{
(snip)
    fdsr = ecalloc(howmany(ttyfd + 1, NFDBITS), sizeof(fd_mask));
    for (;;) {
	FD_SET(ttyfd, fdsr);
	tv.tv_sec = 0;
	tv.tv_usec = 0;

	nready = select(ttyfd + 1, fdsr, NULL, NULL, paused ? NULL : &tv);
------------------------

In this case, __builtin_object_size() doesn't work too. However fixing is easy.
We only need to annotate ecalloc as attribute((alloc_size(1,2))).


rsyslog
-------------------------
BEGINobjConstruct(nsdsel_ptcp) /* be sure to specify the object type also in END macro! */
	pThis->maxfds = 0;
#ifdef USE_UNLIMITED_SELECT
        pThis->pReadfds = calloc(1, glbl.GetFdSetSize());
        pThis->pWritefds = calloc(1, glbl.GetFdSetSize());
#else
	FD_ZERO(&pThis->readfds);
	FD_ZERO(&pThis->writefds);
#endif
ENDobjConstruct(nsdsel_ptcp)
-------------------------

In this case, __builtin_object_size() doesn't work too. because pThis->p{Read,Write}fds can be
changed at runtime. compiler can't distingush the bitmap size.


openssh
------------------------
static int
timeout_connect(int sockfd, const struct sockaddr *serv_addr,
    socklen_t addrlen, int *timeoutp)
{
(snip)
	fdset = (fd_set *)xcalloc(howmany(sockfd + 1, NFDBITS),
	    sizeof(fd_mask));
	FD_SET(sockfd, fdset);
	ms_to_timeval(&tv, *timeoutp);

	for (;;) {
		rc = select(sockfd + 1, NULL, fdset, NULL, &tv);
------------------------


In this case, xcalloc also prevent to work __builtin_object_size(). It should be marded as attribute((alloc_size(1,2))).
but,

openssh 
------------------------
void
channel_prepare_select(fd_set **readsetp, fd_set **writesetp, int *maxfdp,
    u_int *nallocp, time_t *minwait_secs, int rekeying)
{
	u_int n, sz, nfdset;

	n = MAX(*maxfdp, channel_max_fd);

	nfdset = howmany(n+1, NFDBITS);
	/* Explicitly test here, because xrealloc isn't always called */
	if (nfdset && SIZE_T_MAX / nfdset < sizeof(fd_mask))
		fatal("channel_prepare_select: max_fd (%d) is too large", n);
	sz = nfdset * sizeof(fd_mask);

	/* perhaps check sz < nalloc/2 and shrink? */
	if (*readsetp == NULL || sz > *nallocp) {
		*readsetp = xrealloc(*readsetp, nfdset, sizeof(fd_mask));
		*writesetp = xrealloc(*writesetp, nfdset, sizeof(fd_mask));
		*nallocp = sz;
	}
	*maxfdp = n;
	memset(*readsetp, 0, sz);
	memset(*writesetp, 0, sz);

	if (!rekeying)
		channel_handler(channel_pre, *readsetp, *writesetp,
		    minwait_secs);
}
------------------------

This case is more harder. compiler can't know the size of readsetp and writesetp at
compilering caller functions.


librpcsecgss
------------------------
static int
readtcp(ct, buf, len)
	register struct ct_data *ct;
	caddr_t buf;
	register int len;
{
(snip)
	if (ct->ct_sock+1 > FD_SETSIZE) {
		int bytes = howmany(ct->ct_sock+1, NFDBITS) * sizeof(fd_mask);
		fds = (fd_set *)malloc(bytes);
		if (fds == NULL)
			return (-1);
		memset(fds, 0, bytes);
	} else {
		fds = &readfds;
		FD_ZERO(fds);
	}

	gettimeofday(&start, NULL);
	delta = ct->ct_wait;
	while (TRUE) {
		/* XXX we know the other bits are still clear */
		FD_SET(ct->ct_sock, fds);
		r = select(ct->ct_sock+1, fds, NULL, NULL, &delta);
		save_errno = errno;
--------------------------

This is ok. compiler can understand the size of fds.


bind9
-----------------------

static isc_result_t
setup_watcher(isc_mem_t *mctx, isc__socketmgr_t *manager) {
	isc_result_t result;
#if defined(USE_KQUEUE) || defined(USE_EPOLL) || defined(USE_DEVPOLL)
	char strbuf[ISC_STRERRORSIZE];
#endif
(snip)
#if ISC_SOCKET_MAXSOCKETS > FD_SETSIZE
	/*
	 * Note: this code should also cover the case of MAXSOCKETS <=
	 * FD_SETSIZE, but we separate the cases to avoid possible portability
	 * issues regarding howmany() and the actual representation of fd_set.
	 */
	manager->fd_bufsize = howmany(manager->maxsocks, NFDBITS) *
		sizeof(fd_mask);
#else
	manager->fd_bufsize = sizeof(fd_set);
#endif
---------------------------

This case also harder. FD_SET is called far different functions.



Summary: alomost software only need to add alloc_size() annotation to xmalloc() or 
similar in almost case. but there are several exceptions. some software have a complicated
fd size management and can't use __builtin_object_size(). but that's ok. In this case, the
software correctly expand buffers by realloc() or similar, so there is no chance to happen
buffer overflow.