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

[Joakim Tjernlund]

Peter Maydell <address@hidden> wrote on 2014/07/12 17:47:56:

> From: Peter Maydell <address@hidden>
> To: Joakim Tjernlund <address@hidden>, 
> Cc: QEMU Developers <address@hidden>
> Date: 2014/07/12 17:48
> Subject: Re: [Qemu-devel] [PATCH 1/4] qemu-user: Impl. 
setsockopt(SO_BINDTODEVICE)
> 
> On 12 July 2014 16:13, Joakim Tjernlund <address@hidden> 
wrote:
> > Peter Maydell <address@hidden> wrote on 2014/07/11 19:02:30:
> >> >
> >> > 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.
> >
> > hmm, this kernel code:
> >         if (optlen > IFNAMSIZ - 1)
> >                 optlen = IFNAMSIZ - 1;
> >         memset(devname, 0, sizeof(devname));
> >
> >         ret = -EFAULT;
> >         if (copy_from_user(devname, optval, optlen))
> >                 goto out;
> >
> > suggests to me that the qemu code could be written:
> >
> >         case TARGET_SO_BINDTODEVICE:
> >         {
> >                 char *dev_ifname;
> >
> >                 if (optlen > IFNAMSIZ - 1) {
> >                     optlen = IFNAMSIZ - 1;
> >                 }
> >                 dev_ifname = lock_user(VERIFY_READ, optval_addr, 
optlen,
> > 1);
> >                 if (!dev_ifname) {
> >                     return -TARGET_EFAULT;
> >                 }
> >                 optname = SO_BINDTODEVICE;
> >                 ret = get_errno(setsockopt(sockfd, level, optname,
> > dev_ifname, optlen));
> >                 unlock_user (dev_ifname, optval_addr, 0);
> >                 return ret;
> >         }
> >
> > Not a big deal, I just wanted to improve my understanding of the
> > kernel/qemu code.
> 
> That would work with the current kernel implementation, but
> the API definition (as described in the socket(7) manpage)
> says we should pass a NUL-terminated string to the kernel,
> so it's more robust for us to make sure we do so.

But we emulate user space and we should not "improve" stuff on the way 
should we?
If we do then one could end up with progs that work with qemu but not with
the real kernel.

 Jocke