crypto: scatterwalk_copychunks() fails to advance through scatterlist

In the loop in scatterwalk_copychunks(), if walk->offset is zero, then scatterwalk_pagedone rounds that up to the nearest page boundary:

		walk->offset += PAGE_SIZE - 1;
		walk->offset &= PAGE_MASK;

which is a no-op in this case, so we don't advance to the next element of the scatterlist array:

		if (walk->offset >= walk->sg->offset + walk->sg->length)
			scatterwalk_start(walk, sg_next(walk->sg));

and we end up copying the same data twice.

It appears that other callers of scatterwalk_{page}done first advance walk->offset, so I believe that's the correct thing to do here.

This caused a bug in NFS when run with krb5p security, which would cause some writes to fail with permissions errors--for example, writes of less than 8 bytes (the des blocksize) at the start of a file.

A git-bisect shows the bug was originally introduced by 5c64097aa0f6dc4f27718ef47ca9a12538d62860, first in 2.6.19-rc1.

---
 crypto/scatterwalk.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

index 35172d3..a664231 100644
@@ -91,6 +91,8 @@ void scatterwalk_copychunks(void *buf, struct scatter_walk *walk,
 		memcpy_dir(buf, vaddr, len_this_page, out);
 		scatterwalk_unmap(vaddr, out);
 
+		scatterwalk_advance(walk, nbytes);
+
 		if (nbytes == len_this_page)
 			break;
 

@@ -99,7 +101,5 @@ void scatterwalk_copychunks(void *buf, struct scatter_walk *walk,
 
 		scatterwalk_pagedone(walk, out, 1);
 	}
-
-	scatterwalk_advance(walk, nbytes);
 }
 EXPORT_SYMBOL_GPL(scatterwalk_copychunks);

1.5.0.3.31.ge47c

Thanks for the patch. However I still have a question as to why this is happening.

As far as I can see scatterwalk_copychunks is only called in two places. In both spots it only processes one block of data. Since we set the maximum block size to PAGE_SIZE/8 I don't see how you can get an offset of zero and still roll over to the next page in scatterwalk_copychunks.

So perhaps the bug is elsewhere in the original patch?

Your patch looks like a good clean-up anyway but I just wanted to make sure that I understand what the problem is first.

Cheers,

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Are the elements of the scatterlists assumed to always be full pages? I need to encrypt things that look like, for example:

	sg[0].page = page1
	sg[0].offset = 0
	sg[0].length = 5
	sg[1].page = page2
	sg[1].offset = 0
	sg[1].length = 37

and worse.... I could do this by hand if I had to, but the crypto code, if it's not designed to handle this sort of thing, seems very close, so I'd rather enhance it than duplicate a lot of this complicated scatterlist-traversal stuff.

--b.

(Erm, second should be 27--I did mean this to come out to a multiple of 8....)--b.

Definitely not.

OK I see what you mean now. I'll apply your patch.

Thanks,

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Thanks!

By the way, the commit that I believe introduced this regression did two or three different things at once--I spent some time staring at it and can't say I really understand the change. That's probably just me! But would it be possible to split up these patches fine enough that people inexperienced with this code would have a better chance of understanding what's going on?

There's so many potential corner cases to handle here, I know I'd want to tread very carefully....

(And did the crypto testing module use to have some tests for buffers that were fragmented in odd ways, or was I imagining that? I'd be happy to help come up with some test cases if it'd be useful.)

--b.

Good point, I'll make sure they're more granular in future.

It does test fragments but it doesn't do it in the way you suggested. In particular, the fragments probably won't be starting off at a page boundary.

Cheers,

Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt