This is the mail archive of the
libc-ports@sources.redhat.com
mailing list for the libc-ports project.
Re: [PATCH 2/5] __FD_ELT: Implement correct buffer overflow check
- From: KOSAKI Motohiro <kosaki dot motohiro at gmail dot com>
- To: Carlos O'Donell <carlos at redhat dot com>
- Cc: libc-alpha <libc-alpha at sourceware dot org>, "libc-ports at sourceware dot org" <libc-ports at sourceware dot org>, kosaki dot motohiro at gmail dot com
- Date: Wed, 01 May 2013 16:11:40 -0400
- Subject: Re: [PATCH 2/5] __FD_ELT: Implement correct buffer overflow check
- References: <1365900451-19026-1-git-send-email-kosaki dot motohiro at gmail dot com> <1365900451-19026-3-git-send-email-kosaki dot motohiro at gmail dot com> <518080FD dot 1090402 at redhat dot com> <CAHGf_=pDgABHdv5RKd6U870J1t1gM6GhbDpxGoQMjJEsMPHgLQ at mail dot 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.