linux-kernel - Re: [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 9 Dec 2020 04:03:12 +0000
From:   Matthew Wilcox <willy@...radead.org>
To:     Ira Weiny <ira.weiny@...el.com>
Cc:     Dan Williams <dan.j.williams@...el.com>,
        "Darrick J. Wong" <darrick.wong@...cle.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Dave Hansen <dave.hansen@...el.com>,
        Christoph Hellwig <hch@...radead.org>,
        Al Viro <viro@...iv.linux.org.uk>,
        Eric Biggers <ebiggers@...nel.org>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH V2 2/2] mm/highmem: Lift memcpy_[to|from]_page to core

On Tue, Dec 08, 2020 at 06:22:50PM -0800, Ira Weiny wrote:
> Right now we have a mixed bag.  zero_user() [and it's variants, circa 2008]
> does a BUG_ON.[0]  While the other ones do nothing; clear_highpage(),
> clear_user_highpage(), copy_user_highpage(), and copy_highpage().

Erm, those functions operate on the entire PAGE_SIZE.  There's nothing
for them to check.

> While continuing to audit the code I don't see any users who would violating
> the API with a simple conversion of the code.  The calls which I have worked on
> [which is many at this point] all have checks in place which are well aware of
> page boundaries.

Oh good, then this BUG_ON won't trigger.

> Therefore, I tend to agree with Dan that if anything is to be done it should be
> a WARN_ON() which is only going to throw an error that something has probably
> been wrong all along and should be fixed but continue running as before.

Silent data corruption is for ever.  Are you absolutely sure nobody has
done:

	page = alloc_pages(GFP_HIGHUSER_MOVABLE, 3);
	memcpy_to_page(page, PAGE_SIZE * 2, p, PAGE_SIZE * 2);

because that will work fine if the pages come from ZONE_NORMAL and fail
miserably if they came from ZONE_HIGHMEM.

> FWIW I think this is a 'bad BUG_ON' use because we are "checking something that
> we know we might be getting wrong".[1]  And because, "BUG() is only good for
> something that never happens and that we really have no other option for".[2]

BUG() is our only option here.  Both limiting how much we copy or
copying the requested amount result in data corruption or leaking
information to a process that isn't supposed to see it.

What Linus is railing against is the developers who say "Oh, I don't
know what to do here, I'll just BUG()".  That's not the case here.
We've thought about it.  We've discussed it.  There's NO GOOD OPTION.

Unless you want to do the moral equivalent of this:

http://git.infradead.org/users/willy/pagecache.git/commitdiff/d2417516bd8b3dd1db096a9b040b0264d8052339

I think that would look something like this ...

void memcpy_to_page(struct page *page, size_t offset, const char *from,
			size_t len)
{
	page += offset / PAGE_SIZE;
	offset %= PAGE_SIZE;

	while (len) {
		char *to = kmap_atomic(page);
		size_t bytes = min(len, PAGE_SIZE - offset);
		memcpy(to + offset, from, len);
		kunmap_atomic(to);
		len -= bytes;
		offset = 0;
		page++;
	}
}

Now 32-bit highmem will do the same thing as 64-bit for my example above,
just more slowly.  Untested, obviously.