linux-kernel - Re: [PATCH 1/5] pagemap: Modify add_to_pagemap to use copy_to_user instead of put

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]

Message-Id: <1212682810.3953.180.camel@calx>
Date:	Thu, 05 Jun 2008 11:20:10 -0500
From:	Matt Mackall <mpm@...enic.com>
To:	Thomas Tuttle <ttuttle@...gle.com>
Cc:	akpm@...ux-foundation.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/5] pagemap: Modify add_to_pagemap to use copy_to_user
	instead of put_user.


On Thu, 2008-06-05 at 12:01 -0400, Thomas Tuttle wrote:
> On Thu, Jun 5, 2008 at 11:57 AM, Matt Mackall <mpm@...enic.com> wrote:
> >
> > On Thu, 2008-06-05 at 11:04 -0400, Thomas Tuttle wrote:
> >> While fiddling with pagemap, I discovered a bug in add_to_pagemap.
> >> When it is copying an entry that is not at the end of the buffer, it
> >> uses put_user to copy a u64 into a char* buffer.  The problem is that
> >> put_user determines how much to copy based on the size of the
> >> *destination*, not the source, so it only copied one byte.  To fix
> >> this, I replaced the call to put_user with a call to copy_to_user, as
> >> is used when copying the last (possibly partial) PFN into the buffer.0
> >
> > This looks fine to me, so:
> >
> > Acked-by: Matt Mackall <mpm@...enic.com>
> >
> > But your 3/5 undoes this, right? So we should just take one or the other
> > route. While I like going the simplifying route, it's not very pretty
> > from the user interface point of view. But it does have plenty of
> > precedent in direct-I/O-like things.
> 
> No, my 3/5 further keeps the use of copy_to_user instead of put_user,
> but eliminates entirely the code for partial reads.  3/5 depends on
> 1/5.

Hmm, I read this backwards the first time through then. Rather than
finding a small bug at the end of the buffer, you've found a REALLY BIG
WEAR-A-BROWN-PAPER-BAG-ON-MY-HEAD BUG.

Sigh. Some clever person (me?) probably removed a cast or changed a
pointer type somewhere along the way and didn't notice the fallout
because the MSBs still came through.

I'm pretty sure we want to do the put_user because it's faster. On a
64-bit system, it boils down to a simple assignment. If we commit
ourselves to the alignment thing, then we should probably just change
all the relevant pointers to u64, and then it will just do the right
thing.

ps: perhaps put_user can be made to compare the src and dst sizes at
compile time?

-- 
Mathematics is the supreme nostalgia of our time.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/