From: KOSAKI Motohiro <kosaki.motohiro@gmail.com>
To: Carlos O'Donell <carlos@redhat.com>
Cc: libc-alpha <libc-alpha@sourceware.org>,
"libc-ports@sourceware.org" <libc-ports@sourceware.org>,
kosaki.motohiro@gmail.com
Subject: Re: [PATCH 2/5] __FD_ELT: Implement correct buffer overflow check
Date: Wed, 01 May 2013 20:11:00 -0000 [thread overview]
Message-ID: <518176FC.5030906@gmail.com> (raw)
In-Reply-To: <CAHGf_=pDgABHdv5RKd6U870J1t1gM6GhbDpxGoQMjJEsMPHgLQ@mail.gmail.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.
next prev parent reply other threads:[~2013-05-01 20:11 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-04-14 0:47 [PATCH v4 0/5] fix wrong program abort on __FD_ELT KOSAKI Motohiro
2013-04-14 0:47 ` [PATCH 3/5] update libc.abilist KOSAKI Motohiro
2013-04-14 0:47 ` [PATCH 5/5] __FDS_BITS: Added cast to __fd_mask* to avoid warning KOSAKI Motohiro
2013-05-01 2:44 ` Carlos O'Donell
2013-04-14 0:47 ` [PATCH 2/5] __FD_ELT: Implement correct buffer overflow check KOSAKI Motohiro
2013-05-01 2:42 ` Carlos O'Donell
2013-05-01 6:28 ` KOSAKI Motohiro
2013-05-01 14:42 ` Carlos O'Donell
2013-05-01 20:32 ` KOSAKI Motohiro
2013-05-03 3:15 ` Carlos O'Donell
2013-05-01 20:11 ` KOSAKI Motohiro [this message]
2013-05-03 3:15 ` Carlos O'Donell
2013-04-14 0:47 ` [PATCH 4/5] tst-chk1: add fd_set dynamic allocation test KOSAKI Motohiro
2013-05-01 2:44 ` Carlos O'Donell
2013-05-01 6:29 ` KOSAKI Motohiro
2013-04-14 0:47 ` [PATCH 1/5] __fdelt_chk: Removed range check KOSAKI Motohiro
2013-05-01 2:25 ` Carlos O'Donell
2013-05-01 6:40 ` KOSAKI Motohiro
2013-05-01 14:45 ` Carlos O'Donell
2013-05-01 22:13 ` KOSAKI Motohiro
2013-05-03 2:52 ` Carlos O'Donell
2013-05-01 3:08 ` [PATCH v4 0/5] fix wrong program abort on __FD_ELT Carlos O'Donell
2013-05-01 5:31 ` KOSAKI Motohiro
2013-05-01 14:38 ` Carlos O'Donell
2013-05-01 22:21 ` KOSAKI Motohiro
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=518176FC.5030906@gmail.com \
--to=kosaki.motohiro@gmail.com \
--cc=carlos@redhat.com \
--cc=libc-alpha@sourceware.org \
--cc=libc-ports@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).