Re: [Qemu-devel] [PATCH 1/4] qemu-user: Impl. setsockopt(SO_BINDTODEVICE)

[Peter Maydell]

On 11 July 2014 17:07, Joakim Tjernlund <address@hidden> wrote:
> Peter Maydell <address@hidden> wrote on 2014/07/11 17:46:28:
>> This needs braces for our coding style; you might like
>> to run your patches through scripts/checkpatch.pl, which
>> will warn about this kind of thing.
>
> Ahh, I figured you had kernel style.

No, we're a bit different (and we don't always follow
our own style in existing code, though we try to
maintain it on new changes).

>>
>> Also, the kernel implementation handles overlong
>> lengths via
>>      if (optlen > IFNAMSIZ - 1) {
>>          optlen = IFNAMSIZ - 1;
>>      }
>>
>> which effectively truncates overlong strings rather
>> than rejecting them. We should do the same (there will
>> be complication required to ensure we pass the kernel
>> a NUL-terminated string even if we don't get one from
>> the guest; may be easiest to use a local array, given
>> IFNAMSIZ is only 16).
>
> hmm, should we not pass through as is to the kernel?
> Since we don't copy anything we could just remove this
> check and let the kernel decide policy?

I thought about that, but there's a corner case:
the kernel does the clamping of the optlen before the
copy_from_user(), which means if you have:
 [interface name] [unreadable memory]
and optlen is long enough that optval_addr + optlen
reaches into the unreadable memory, then QEMU will return
EFAULT (whereas the native kernel implementation will
succeed) unless we do the clamping of the optlen ourselves.

Which reminds me that your patch forgot the check
    if (!dev_ifname) {
        return -TARGET_EFAULT;
    }
(lock_user can fail).

thanks
-- PMM