On 2021/11/5 07:48, Linus Torvalds wrote:
On Thu, Nov 4, 2021 at 4:37 PM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
Having looked at it once more, it still looks "ObviouslyCorrect(tm)"
to me, but I suspect I'm just being blind to some obvious bug.
Oh, I was just looking at the pattern of kmap/kunmap, but there does
seem to be a questionable pattern outside of that:
This pattern looks odd:
kaddr = kmap(cur_page);
write_compress_length(kaddr + offset_in_page(*cur_out),
compressed_size);
...
(and then whether you kunmap immediately, or you leave it kmap'ed and
use it again at the end is a different issue)
That part is paired with the the following code, to prevent we cross
page boundary for the segment header:
/*
* Check if we can fit the next segment header into the remaining space
* of the sector.
*/
sector_bytes_left = round_up(*cur_out, sectorsize) - *cur_out;
if (sector_bytes_left >= LZO_LEN || sector_bytes_left == 0)
goto out;
/* The remaining size is not enough, pad it with zeros */
memset(kaddr + offset_in_page(*cur_out), 0,
sector_bytes_left);
*cur_out += sector_bytes_left;
So we always ensure that each segment header never crosses the page
boundary.
This behavior is a little tricky but is part of the on-disk format for
lzo compressed data.
BTW, I also thought that part can be suspicious, as it keeps the page
mapped (unlike all other call sites), thus I tried the following diff,
but no difference for the leakage:
diff --git a/fs/btrfs/lzo.c b/fs/btrfs/lzo.c
index 65cb0766e62d..0648acc48310 100644
--- a/fs/btrfs/lzo.c
+++ b/fs/btrfs/lzo.c
@@ -151,6 +151,7 @@ static int copy_compressed_data_to_page(char
*compressed_data,
kaddr = kmap(cur_page);
write_compress_length(kaddr + offset_in_page(*cur_out),
compressed_size);
+ kunmap(cur_page);
*cur_out += LZO_LEN;
orig_out = *cur_out;
@@ -160,7 +161,6 @@ static int copy_compressed_data_to_page(char
*compressed_data,
u32 copy_len = min_t(u32, sectorsize - *cur_out % sectorsize,
orig_out + compressed_size - *cur_out);
- kunmap(cur_page);
cur_page = out_pages[*cur_out / PAGE_SIZE];
/* Allocate a new page */
if (!cur_page) {
@@ -173,6 +173,7 @@ static int copy_compressed_data_to_page(char
*compressed_data,
memcpy(kaddr + offset_in_page(*cur_out),
compressed_data + *cur_out - orig_out, copy_len);
+ kunmap(cur_page);
*cur_out += copy_len;
}
@@ -186,12 +187,15 @@ static int copy_compressed_data_to_page(char
*compressed_data,
goto out;
/* The remaining size is not enough, pad it with zeros */
+ cur_page = out_pages[*cur_out / PAGE_SIZE];
+ ASSERT(cur_page);
+ kmap(cur_page);
memset(kaddr + offset_in_page(*cur_out), 0,
sector_bytes_left);
+ kunmap(cur_page);
*cur_out += sector_bytes_left;
out:
- kunmap(cur_page);
return 0;
}
Thanks,
Qu
In particular, what if "offset_in_page(*cur_out)" is very close to the
end of the page?
That write_compress_length() always writes out a word-sized length (ie
LZO_LEN bytes), and the above pattern seems to have no model to handle
the "oh, this 4-byte write crosses a page boundary"
The other writes in that function seem to do it properly, and you have
u32 copy_len = min_t(u32, sectorsize - *cur_out % sectorsize,
orig_out + compressed_size - *cur_out);
so doing the memcpy() of size 'copy_len' should never cross a page
boundary as long as sectorsize is a power-of-2 smaller or equal to a
page size. But those 4-byte length writes seem like they could be page
crossers.
The same situation exists on the reading side, I think.
Maybe there's some reason why the read/write_compress_length() can
never cross a page boundary, but it did strike me as odd.
Linus
