On Wed, Sep 23, 2020 at 03:16:54PM +0100, Al Viro wrote:
> On Wed, Sep 23, 2020 at 08:05:41AM +0200, Christoph Hellwig wrote:
>
> > +struct iovec *iovec_from_user(const struct iovec __user *uvec,
> > + unsigned long nr_segs, unsigned long fast_segs,
>
> Hmm... For fast_segs unsigned long had always been ridiculous
> (4G struct iovec on caller stack frame?), but that got me wondering about
> nr_segs and I wish I'd thought of that when introducing import_iovec().
>
> The thing is, import_iovec() takes unsigned int there. Which is fine
> (hell, the maximal value that can be accepted in 1024), except that
> we do pass unsigned long syscall argument to it in some places.
>
> E.g. vfs_readv() quietly truncates vlen to 32 bits, and vlen can
> come unchanged through sys_readv() -> do_readv() -> vfs_readv().
> With unsigned long passed by syscall glue.
>
> AFAICS, passing 4G+1 as the third argument to readv(2) on 64bit box
> will be quietly treated as 1 these days. Which would be fine, except
> that before "switch {compat_,}do_readv_writev() to {compat_,}import_iovec()"
> it used to fail with -EINVAL.
>
> Userland, BTW, describes readv(2) iovcnt as int; process_vm_readv(),
> OTOH, has these counts unsigned long from the userland POV...
>
> I suppose we ought to switch import_iovec() to unsigned long for nr_segs ;-/
> Strictly speaking that had been a userland ABI change, even though nothing
> except regression tests checking for expected errors would've been likely
> to notice. And it looks like no regression tests covered that one...
>
> Linus, does that qualify for your "if no userland has noticed the change,
> it's not a breakage"?
Egads... We have sys_readv() with unsigned long for file descriptor, since
1.3.31 when it had been introduced. And originally it did comparison with
NR_OPEN right in sys_readv(). Then in 2.1.60 it had been switched to
fget(), which used to take unsigned long at that point. And in 2.1.90pre1
it went unsigned int, so non-zero upper 32 bits in readv(2) first argument
ceased to cause EBADF...
Of course, libc had it as int fd all along.
