CHROMIUM: verity: don't call page_address on unmapped highmem pages

CHROMIUM: verity: don't call page_address on unmapped highmem pages

This fixes a bug that is preventing ARM systems from booting with verity.

We were calling page_address on what could potentially have been an
unmapped highmem page. Fix is to pass the struct page to the crypto code
and let crypto handle the kmap/kunmap.

BUG=chromium-os:14089, chrome-os-partner:3287
TEST=Ran dm-verity.git unit tests. Ran platform_DMVerityCorruption on H/W.
Verified that ARM now boots.

Change-Id: Id2074052aab9eec8fa0aead39aded2b40cd19bad
Signed-off-by: Mandeep Singh Baines <msb@chromium.org>;

R=wad@chromium.org,olofj@chromium.org,taysom@chromium.org

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=76c7a42

[Will Drewry, 2011-04-13 20:19:30]

Thanks for tracking this down! A few questions, but looks good!

http://codereview.chromium.org/6835032/diff/1/drivers/md/dm-bht.c
File drivers/md/dm-bht.c (right):

http://codereview.chromium.org/6835032/diff/1/drivers/md/dm-bht.c#newcode105
drivers/md/dm-bht.c:105: sg_set_page(&sg, pg, PAGE_SIZE, 0);
should 0 be the new offset argument? Then the comment could be /* TODO:
BUG_ON(offset != 0); */?

http://codereview.chromium.org/6835032/diff/1/drivers/md/dm-bht.c#newcode539
drivers/md/dm-bht.c:539: entry->nodes = page_address(node_page);
Will this call be a problem?

http://codereview.chromium.org/6835032/diff/1/drivers/md/dm-bht.c#newcode674
drivers/md/dm-bht.c:674: pg = virt_to_page(entry->nodes);
Hrm should we yet-again convert nodes back to a struct page, or is virt_to_page
safewhile page_address isn;t?

http://codereview.chromium.org/6835032/diff/1/include/linux/dm-bht.h
File include/linux/dm-bht.h (right):

http://codereview.chromium.org/6835032/diff/1/include/linux/dm-bht.h#newcode130
include/linux/dm-bht.h:130: struct page *pg, unsigned int offset);
nit: Do you need #include <asm/page.h> or will mempool.h (or whatever) pull in
the struct for you?

[Paul T, 2011-04-13 20:47:55]

See comment.

http://codereview.chromium.org/6835032/diff/1/drivers/md/dm-bht.c
File drivers/md/dm-bht.c (right):

http://codereview.chromium.org/6835032/diff/1/drivers/md/dm-bht.c#newcode539
drivers/md/dm-bht.c:539: entry->nodes = page_address(node_page);
This must assume the page is mapped. If it is not, this will be a problem. I
thought the crypto code was doing the mapping/unmapping calls.

[Mandeep Singh Baines, 2011-04-13 21:11:16]

Fixed. PTAL.

http://codereview.chromium.org/6835032/diff/1/drivers/md/dm-bht.c
File drivers/md/dm-bht.c (right):

http://codereview.chromium.org/6835032/diff/1/drivers/md/dm-bht.c#newcode539
drivers/md/dm-bht.c:539: entry->nodes = page_address(node_page);
On 2011/04/13 20:19:30, Will Drewry wrote:
> Will this call be a problem?

I clarified the description. page_address is OK on a highmem page as long as its
mapped.

This call is OK because we didn't explicitly ask for __GFP_HIGHMEM. By default,
you do not get HIGHMEM pages.

http://codereview.chromium.org/6835032/diff/1/drivers/md/dm-bht.c#newcode674
drivers/md/dm-bht.c:674: pg = virt_to_page(entry->nodes);
On 2011/04/13 20:19:30, Will Drewry wrote:
> Hrm should we yet-again convert nodes back to a struct page, or is
virt_to_page
> safewhile page_address isn;t?

The virt_to_page here is safe because we did not use __GFP_HIGHMEM in our
allocation.

[Olof Johansson, 2011-04-13 21:11:57]

Comments below. Mostly worried about hardcoded 0-offsets that can easily be
computed (even if they will be computed to 0 with current implementation).

http://codereview.chromium.org/6835032/diff/1/drivers/md/dm-bht.c
File drivers/md/dm-bht.c (right):

http://codereview.chromium.org/6835032/diff/1/drivers/md/dm-bht.c#newcode770
drivers/md/dm-bht.c:770: dm_bht_compute_hash(bht, virt_to_page(block_data), 0,
Currently block_data is guaranteed to be page aligned, but it'd still be nice to
do the math on offset instead of pass 0, to avoid surprises later -- especially
if/when blocksize smaller than pagesize is used.

http://codereview.chromium.org/6835032/diff/1/drivers/md/dm-bht.c#newcode843
drivers/md/dm-bht.c:843: r = dm_bht_compute_hash(bht, pg, 0, digest);
Same here, should probably compute offset?

[Mandeep Singh Baines, 2011-04-13 21:14:03]

On 2011/04/13 20:47:55, Paul T wrote:
> See comment.
> 
> http://codereview.chromium.org/6835032/diff/1/drivers/md/dm-bht.c
> File drivers/md/dm-bht.c (right):
> 
> http://codereview.chromium.org/6835032/diff/1/drivers/md/dm-bht.c#newcode539
> drivers/md/dm-bht.c:539: entry->nodes = page_address(node_page);
> This must assume the page is mapped. If it is not, this will be a problem. I
> thought the crypto code was doing the mapping/unmapping calls.

Since the page is not allocated with __GFP_HIGHMEM its guaranteed to be mapped.

[Mandeep Singh Baines, 2011-04-13 21:16:03]

On 2011/04/13 21:11:57, Olof Johansson wrote:
> Comments below. Mostly worried about hardcoded 0-offsets that can easily be
> computed (even if they will be computed to 0 with current implementation).
> 
> http://codereview.chromium.org/6835032/diff/1/drivers/md/dm-bht.c
> File drivers/md/dm-bht.c (right):
> 
> http://codereview.chromium.org/6835032/diff/1/drivers/md/dm-bht.c#newcode770
> drivers/md/dm-bht.c:770: dm_bht_compute_hash(bht, virt_to_page(block_data), 0,
> Currently block_data is guaranteed to be page aligned, but it'd still be nice
to
> do the math on offset instead of pass 0, to avoid surprises later --
especially
> if/when blocksize smaller than pagesize is used.
> 
> http://codereview.chromium.org/6835032/diff/1/drivers/md/dm-bht.c#newcode843
> drivers/md/dm-bht.c:843: r = dm_bht_compute_hash(bht, pg, 0, digest);
> Same here, should probably compute offset?

Discussed with olofj verbally.

There are still a lot of assumptions about block size == PAGE_SIZE. My plan is
to clean these up in a later phase where I remove all these assumptions.

[Olof Johansson, 2011-04-13 21:18:41]

On 2011/04/13 21:16:03, Mandeep Singh Baines wrote:
> There are still a lot of assumptions about block size == PAGE_SIZE. My plan is
> to clean these up in a later phase where I remove all these assumptions.

Given that there are current, ongoing, efforts to clean this up, this is OK with
me. If not, I would be wary -- the risk for someone not noticing the hardcoded 0
and having to hunt a bug when changing unrelated code would be too large.

So, with that reservation: LGTM.

[taysom, 2011-04-13 21:22:53]

LGTM

On Wed, Apr 13, 2011 at 2:18 PM, <olofj@chromium.org> wrote:

> On 2011/04/13 21:16:03, Mandeep Singh Baines wrote:
>
>> There are still a lot of assumptions about block size == PAGE_SIZE. My
>> plan is
>> to clean these up in a later phase where I remove all these assumptions.
>>
>
> Given that there are current, ongoing, efforts to clean this up, this is OK
> with
> me. If not, I would be wary -- the risk for someone not noticing the
> hardcoded 0
> and having to hunt a bug when changing unrelated code would be too large.
>
> So, with that reservation: LGTM.
>
>
> http://codereview.chromium.org/6835032/
>

[Will Drewry, 2011-04-13 21:37:27]

On 2011/04/13 21:18:41, Olof Johansson wrote:
> On 2011/04/13 21:16:03, Mandeep Singh Baines wrote:
> > There are still a lot of assumptions about block size == PAGE_SIZE. My plan
is
> > to clean these up in a later phase where I remove all these assumptions.
> 
> Given that there are current, ongoing, efforts to clean this up, this is OK
with
> me. If not, I would be wary -- the risk for someone not noticing the hardcoded
0
> and having to hunt a bug when changing unrelated code would be too large.
> 
> So, with that reservation: LGTM.

Ditto. LGTM.  Thanks for tracking this down!