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

[Joakim Tjernlund]

Peter Maydell <address@hidden> wrote on 2014/07/11 19:02:30:

> From: Peter Maydell <address@hidden>
> To: Joakim Tjernlund <address@hidden>, 
> Cc: QEMU Developers <address@hidden>
> Date: 2014/07/11 19:02
> Subject: Re: [Qemu-devel] [PATCH 1/4] qemu-user: Impl. 
setsockopt(SO_BINDTODEVICE)
> 
Snip
> 
> >>
> >> 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.

I can live with that IMHO very minor flaw that I dont think is
going to matter in practice for simplicity and speed of QEMU.
It is your call though, do we go for exact emulation or can we
cut some corners?

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