Re: [Qemu-devel] [PATCH] add fd limitations for avoiding a buffer overflow

[Markus Armbruster]

Laszlo Ersek <address@hidden> writes:

> On 01/25/13 09:14, Amos Kong wrote:
>> FD_SET() and FD_CLR() are used to add and remove one descriptor from a
>> set, the 'fd' should be less than FD_SETSIZE. Glibc will give a warning
>> and crash the qemu when we set a fd (1024) to a set.
>> 
>>  # qemu -device virtio-net-pci,netdev=macvtap_netdev,mac=92:ff:8a:11:fe:57
>>    -netdev tap,id=macvtap_netdev,fd=1024 1024<>/dev/tap4
>> 
>> *** buffer overflow detected ***: x86_64-softmmu/qemu-system-x86_64
>> terminated
>> ======= Backtrace: =========
>> /lib64/libc.so.6(__fortify_fail+0x37)[0x7f842a2134a7]
>> /lib64/libc.so.6(+0x35e9d08620)[0x7f842a211620]
>> /lib64/libc.so.6(+0x35e9d0a417)[0x7f842a213417]
>> x86_64-softmmu/qemu-system-x86_64(+0x1901fd)[0x7f842f09f1fd]
>> x86_64-softmmu/qemu-system-x86_64(+0x198388)[0x7f842f0a7388]
>> x86_64-softmmu/qemu-system-x86_64(main+0xfa9)[0x7f842ef897a9]
>> /lib64/libc.so.6(__libc_start_main+0xf5)[0x7f842a12aa05]
>> x86_64-softmmu/qemu-system-x86_64(+0x7ed49)[0x7f842ef8dd49]
>> ======= Memory map: ========
>> ....
>> 
>> This patch added limitations when init tap device and set fd handler
>> for synchronous IO.
>> 
>> Signed-off-by: Amos Kong <address@hidden>
>> ---
>>  iohandler.c |    3 +++
>>  net/tap.c   |    3 ++-
>>  2 files changed, 5 insertions(+), 1 deletions(-)
>> 
>> diff --git a/iohandler.c b/iohandler.c
>> index 2523adc..c22edab 100644
>> --- a/iohandler.c
>> +++ b/iohandler.c
>> @@ -66,6 +66,9 @@ int qemu_set_fd_handler2(int fd,
>>              }
>>          }
>>      } else {
>> +        if (fd >= FD_SETSIZE) {
>> +            return 1;
>> +        }
>
> qemu_set_fd_handler2() -- and consequently, qemu_set_fd_handler() --
> could never fail before. I tried to check their call sites, and most of
> those don't bother to check for the return value; they assume these
> functions always succeed.
>
> Wouldn't it be better to abort() here (or exit with an error message)
> instead of returning 1? (Not suggesting, just asking.)

Never check for an error you don't know how to handle ;-)

What we've done for other functions where some callers can't be bothered
to check for errors[*] is to create a wrapper void FOO_nofail() around
int FOO() that aborts on error, then slap QEMU_WARN_UNUSED_RESULT onto
FOO(), and fix the resulting warnings, either by adding error handling,
or by switching to FOO_nofail().


[*] Sometimes even legitimately, because we *know* errors can't happen.