lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <Y4YGLBZoXKj6broy@ZenIV>
Date:   Tue, 29 Nov 2022 13:16:28 +0000
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Hillf Danton <hdanton@...a.com>
Cc:     syzbot <syzbot+8c7a4ca1cc31b7ce7070@...kaller.appspotmail.com>,
        akpm@...ux-foundation.org, dan.j.williams@...el.com, hch@....de,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-mm@...ck.org, syzkaller-bugs@...glegroups.com,
        willy@...radead.org
Subject: Re: [syzbot] WARNING in iov_iter_revert (3)

On Tue, Nov 29, 2022 at 12:20:39PM +0000, Al Viro wrote:
> ->direct_IO() should return the amount of data actually copied to userland;
> if that's how much it has consumed from iterator - great, iov_iter_revert(i, 0)
> is a no-op.  If it has consumed more, the caller will take care of that.
> If it has consumed say 4Kb of data from iterator, but claims that it has
> managed to store 12Kb into that, it's broken and should be fixed.
> 
> If it wants to do revert on its own, for whatever reason, it is welcome - nothing
> will break, as long as you do *not* return the value greater than the amount you
> ended up taking from iterator.  However, I don't understand the reason why ntfs3
> wants to bother (and appears to get it wrong, at that); the current rules are
> such that caller will take care of revert.

Looking at ntfs3, WTF does it bother with zeroing on short reads (?) in the
first place?  Anyway, immediate bug there is the assumption that
blockdev_direct_IO() won't consume more than its return value; it bloody
well might.

*IF* you want that logics on reads (again, I'm not at all sure what is it
doing there), you want this:

        } else if (vbo < valid && valid < end) {
		size_t consumed = iter_count - iov_iter_count(iter);
		size_t valid_bytes = valid - vbo;
		iov_iter_revert(iter, consumed - valid_bytes);
		iov_iter_zero(ret - valid_bytes, iter);
	}

This iov_iter_zero() would better not be an attempt to overwrite some
data that shouldn't have been copied to userland; if that's what it
is, it's an infoleak - another thread might have observed the data
copied there before that zeroing.

Oh, and
                if (end > valid && !S_ISBLK(inode->i_mode)) {

several lines above is obvious bollocks - if inode is a block device,
we won't be going anywhere near any NTFS address_space_operations or
ntfs_direct_IO().

Seriously, what's going on with zeroing there?

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ