public inbox for libc-ports@sourceware.org
 help / color / mirror / Atom feed
From: KOSAKI Motohiro <kosaki.motohiro@gmail.com>
To: Carlos O'Donell <carlos@redhat.com>
Cc: KOSAKI Motohiro <kosaki.motohiro@gmail.com>,
	 libc-alpha <libc-alpha@sourceware.org>,
	"libc-ports@sourceware.org" <libc-ports@sourceware.org>
Subject: Re: [PATCH 2/5] __FD_ELT: Implement correct buffer overflow check
Date: Wed, 01 May 2013 20:32:00 -0000	[thread overview]
Message-ID: <51817BBB.5010104@gmail.com> (raw)
In-Reply-To: <518129C7.2020808@redhat.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.
> 
> Then we need a flag, and ruby needs to use the flag to disable the
> check on Linux.
>  
> The fundamental truth is that glibc implements POSIX, not "Linux."
> And in POSIX there is a limit of FD_SETSIZE.
> 
> The default checking should be for POSIX.
> 
> We should provide a way to disable _FORTIFY_SOURCE checks that
> are POSIX-only.
> 
> I still think your current macro is *better* because if __bos0
> works then you have a dynamic check that is better than a static
> check.
> 
> Thus the final solution is a combination of your new __bos0
> changes and a flag to disable the check in the event that __bos0
> fails.
> 
> What do you think?

Hmmm....

I'm puzzuled why you started to talk about ruby again. In ruby case,
recompilation and flag are ok. That's no problem.

But, as we've alread seen, several other software also uses the same technique.
and if not disable, Ubuntu need to recompile all of their packages. Do you
suggest to recompile all?

Moreover, IMHO fallbacking static check is completely useless because compiler
always can know the exact buffer size when using fd_set on stack. That's easy task
to distingush static array size form point of compiler view. In the other
hands, if compipler need to fall back, the buffer was allocated from heap in 99% 
case. and when using buffers allocated from heap, the size is larger than 1024
in almost all case. Then evetually, static check fallbacks makes false positive
aborting in almost all case.

Do you disagree?

I guess my conservative and your conservative are slightly different. My conservative
meant not to make false positive aborting. Your conservative seems preserve old behavior
as far as possible. In general, I agree with you. but in this case, I don't think __bos0()
fails to preserve to detect wrong FD_SET usage for buffers on stack. Do you have any 
specific (and practical) examples that my code fails to work?

# I know several hacky code _can_ trick my code. but I have not found practical and real world
# example.


But again, It's ok from ruby POV and I'm not argue if you really want to do it.



  reply	other threads:[~2013-05-01 20:32 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 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-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 [this message]
2013-05-03  3:15           ` Carlos O'Donell
2013-05-01 20:11       ` KOSAKI Motohiro
2013-05-03  3:15         ` 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=51817BBB.5010104@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).