[PATCH U-BOOT v2 0/3] fs: btrfs: Fix false LZO decompression error due to missing page boundary check
There is a bug that uboot can't load LZO compressed data extent while kernel can handle it without any problem.
It turns out to be a page boundary case. The 3nd patch is the proper fix, cross-ported from btrfs-progs.
The first patch is just to make my eyes less hurt. The second patch is to make sure the driver will reject sector size not matching PAGE_SIZE. This keeps the behavior the same as kernel, even in theory we could do better in U-boot. This is just a temporary fix, before better btrfs driver implemented.
I guess it's time to backport proper code from btrfs-progs, other than using tons of immediate codes.
Changelog: v2: - Fix code style problems - Add a new patch to reject non-page-sized sector size Since kernel does the same thing, and non-4K page size u-boot boards are really rare, it shouldn't be a big problem.
Qu Wenruo (3): fs: btrfs: Use LZO_LEN to replace immediate number fs: btrfs: Reject fs with sector size other than PAGE_SIZE fs: btrfs: Fix LZO false decompression error caused by pending zero
fs/btrfs/compression.c | 42 +++++++++++++++++++++++++++++++----------- fs/btrfs/super.c | 8 ++++++++ 2 files changed, 39 insertions(+), 11 deletions(-)
Just a cleanup. These immediate numbers make my eyes hurt.
Signed-off-by: Qu Wenruo wqu@suse.com Cc: Marek Behun marek.behun@nic.cz --- fs/btrfs/compression.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c index 346875d45a1b..4ef44ce11485 100644 --- a/fs/btrfs/compression.c +++ b/fs/btrfs/compression.c @@ -12,36 +12,38 @@ #include <u-boot/zlib.h> #include <asm/unaligned.h>
+/* Header for each segment, LE32, recording the compressed size */ +#define LZO_LEN 4 static u32 decompress_lzo(const u8 *cbuf, u32 clen, u8 *dbuf, u32 dlen) { u32 tot_len, in_len, res; size_t out_len; int ret;
- if (clen < 4) + if (clen < LZO_LEN) return -1;
tot_len = le32_to_cpu(get_unaligned((u32 *)cbuf)); - cbuf += 4; - clen -= 4; - tot_len -= 4; + cbuf += LZO_LEN; + clen -= LZO_LEN; + tot_len -= LZO_LEN;
if (tot_len == 0 && dlen) return -1; - if (tot_len < 4) + if (tot_len < LZO_LEN) return -1;
res = 0;
- while (tot_len > 4) { + while (tot_len > LZO_LEN) { in_len = le32_to_cpu(get_unaligned((u32 *)cbuf)); - cbuf += 4; - clen -= 4; + cbuf += LZO_LEN; + clen -= LZO_LEN;
- if (in_len > clen || tot_len < 4 + in_len) + if (in_len > clen || tot_len < LZO_LEN + in_len) return -1;
- tot_len -= 4 + in_len; + tot_len -= (LZO_LEN + in_len);
out_len = dlen; ret = lzo1x_decompress_safe(cbuf, in_len, dbuf, &out_len);
On Thu, 26 Mar 2020 13:35:54 +0800 Qu Wenruo wqu@suse.com wrote:
Just a cleanup. These immediate numbers make my eyes hurt.
Signed-off-by: Qu Wenruo wqu@suse.com Cc: Marek Behun marek.behun@nic.cz
fs/btrfs/compression.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)
diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c index 346875d45a1b..4ef44ce11485 100644 --- a/fs/btrfs/compression.c +++ b/fs/btrfs/compression.c @@ -12,36 +12,38 @@ #include <u-boot/zlib.h> #include <asm/unaligned.h>
+/* Header for each segment, LE32, recording the compressed size */ +#define LZO_LEN 4 static u32 decompress_lzo(const u8 *cbuf, u32 clen, u8 *dbuf, u32 dlen) { u32 tot_len, in_len, res; size_t out_len; int ret;
- if (clen < 4)
if (clen < LZO_LEN) return -1;
tot_len = le32_to_cpu(get_unaligned((u32 *)cbuf));
- cbuf += 4;
- clen -= 4;
- tot_len -= 4;
cbuf += LZO_LEN;
clen -= LZO_LEN;
tot_len -= LZO_LEN;
if (tot_len == 0 && dlen) return -1;
- if (tot_len < 4)
if (tot_len < LZO_LEN) return -1;
res = 0;
- while (tot_len > 4) {
- while (tot_len > LZO_LEN) { in_len = le32_to_cpu(get_unaligned((u32 *)cbuf));
cbuf += 4;clen -= 4;
cbuf += LZO_LEN;clen -= LZO_LEN;
if (in_len > clen || tot_len < 4 + in_len)
if (in_len > clen || tot_len < LZO_LEN + in_len) return -1;
tot_len -= 4 + in_len;
tot_len -= (LZO_LEN + in_len);out_len = dlen; ret = lzo1x_decompress_safe(cbuf, in_len, dbuf, &out_len);
Reviewed-by: Marek Behún marek.behun@nic.cz
On Thu, Mar 26, 2020 at 01:35:54PM +0800, Qu Wenruo wrote:
Just a cleanup. These immediate numbers make my eyes hurt.
Signed-off-by: Qu Wenruo wqu@suse.com Cc: Marek Behun marek.behun@nic.cz Reviewed-by: Marek Behún marek.behun@nic.cz
Applied to u-boot/master, thanks!
Although in theory u-boot fs driver could easily support more sector sizes, current code base doesn't have good enough way to grab sector size yet.
This would cause problem for later LZO fixes which rely on sector size.
And considering that most u-boot boards are using 4K page size, which is also the most common sector size for btrfs, rejecting fs with non-page-sized sector size shouldn't cause much problem.
This should only be a quick fix before we implement better sector size support.
Signed-off-by: Qu Wenruo wqu@suse.com Cc: Marek Behun marek.behun@nic.cz --- fs/btrfs/super.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index 2dc4a6fcd7a3..b693a073fc0b 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -7,6 +7,7 @@
#include "btrfs.h" #include <memalign.h> +#include <linux/compat.h>
#define BTRFS_SUPER_FLAG_SUPP (BTRFS_HEADER_FLAG_WRITTEN \ | BTRFS_HEADER_FLAG_RELOC \ @@ -232,6 +233,13 @@ int btrfs_read_superblock(void) return -1; }
+ if (sb->sectorsize != PAGE_SIZE) { + printf( + "%s: Unsupported sector size (%u), only supports %u as sector size\n", + __func__, sb->sectorsize, PAGE_SIZE); + return -1; + } + if (btrfs_info.sb.num_devices != 1) { printf("%s: Unsupported number of devices (%lli). This driver " "only supports filesystem on one device.\n", __func__,
On Thu, 26 Mar 2020 13:35:55 +0800 Qu Wenruo wqu@suse.com wrote:
Although in theory u-boot fs driver could easily support more sector sizes, current code base doesn't have good enough way to grab sector size yet.
This would cause problem for later LZO fixes which rely on sector size.
And considering that most u-boot boards are using 4K page size, which is also the most common sector size for btrfs, rejecting fs with non-page-sized sector size shouldn't cause much problem.
This should only be a quick fix before we implement better sector size support.
Signed-off-by: Qu Wenruo wqu@suse.com Cc: Marek Behun marek.behun@nic.cz
fs/btrfs/super.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index 2dc4a6fcd7a3..b693a073fc0b 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -7,6 +7,7 @@
#include "btrfs.h" #include <memalign.h> +#include <linux/compat.h>
#define BTRFS_SUPER_FLAG_SUPP (BTRFS_HEADER_FLAG_WRITTEN \ | BTRFS_HEADER_FLAG_RELOC \ @@ -232,6 +233,13 @@ int btrfs_read_superblock(void) return -1; }
- if (sb->sectorsize != PAGE_SIZE) {
printf(- "%s: Unsupported sector size (%u), only supports %u as sector size\n",
__func__, sb->sectorsize, PAGE_SIZE);return -1;- }
- if (btrfs_info.sb.num_devices != 1) { printf("%s: Unsupported number of devices (%lli). This driver " "only supports filesystem on one device.\n", __func__,
Reviewed-by: Marek Behún marek.behun@nic.cz
On Thu, Mar 26, 2020 at 01:35:55PM +0800, Qu Wenruo wrote:
Although in theory u-boot fs driver could easily support more sector sizes, current code base doesn't have good enough way to grab sector size yet.
This would cause problem for later LZO fixes which rely on sector size.
And considering that most u-boot boards are using 4K page size, which is also the most common sector size for btrfs, rejecting fs with non-page-sized sector size shouldn't cause much problem.
This should only be a quick fix before we implement better sector size support.
Signed-off-by: Qu Wenruo wqu@suse.com Cc: Marek Behun marek.behun@nic.cz Reviewed-by: Marek Behún marek.behun@nic.cz
Applied to u-boot/master, thanks!
For certain btrfs files with compressed file extent, uboot will fail to load it:
btrfs_read_extent_reg: disk_bytenr=14229504 disk_len=73728 offset=0 nr_bytes=131 072 decompress_lzo: tot_len=70770 decompress_lzo: in_len=1389 decompress_lzo: in_len=2400 decompress_lzo: in_len=3002 decompress_lzo: in_len=1379 decompress_lzo: in_len=88539136 decompress_lzo: header error, in_len=88539136 clen=65534 tot_len=62580
NOTE: except the last line, all other lines are debug output.
Btrfs lzo compression uses its own format to record compressed size (segment header, LE32).
However to make decompression easier, we never put such segment header across page boundary.
In above case, the xxd dump of the lzo compressed data looks like this:
00001fe0: 4cdc 02fc 0bfd 02c0 dc02 0d13 0100 0001 L............... 00001ff0: 0000 0008 0300 0000 0000 0011 0000|0000 ................ 00002000: 4705 0000 0001 cc02 0000 0000 0000 1e01 G...............
'|' is the "expected" segment header start position.
But in that page, there are only 2 bytes left, can't contain the 4 bytes segment header.
So btrfs compression will skip that 2 bytes, put the segment header in next page directly.
Uboot doesn't have such check, and read the header with 2 bytes offset, result 0x05470000 (88539136), other than the expected result 0x00000547 (1351), resulting above error.
Follow the btrfs-progs restore implementation, by introducing tot_in to record total processed bytes (including headers), and do proper page boundary skip to fix it.
Please note that, current code base doesn't parse fs_info thus we can't grab sector size easily, so it uses PAGE_SIZE, and relying on fs open time check to exclude unsupported sector size.
Signed-off-by: Qu Wenruo wqu@suse.com Cc: Marek Behun marek.behun@nic.cz --- fs/btrfs/compression.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c index 4ef44ce11485..b1884fc15ee0 100644 --- a/fs/btrfs/compression.c +++ b/fs/btrfs/compression.c @@ -9,6 +9,7 @@ #include <malloc.h> #include <linux/lzo.h> #include <linux/zstd.h> +#include <linux/compat.h> #include <u-boot/zlib.h> #include <asm/unaligned.h>
@@ -16,7 +17,7 @@ #define LZO_LEN 4 static u32 decompress_lzo(const u8 *cbuf, u32 clen, u8 *dbuf, u32 dlen) { - u32 tot_len, in_len, res; + u32 tot_len, tot_in, in_len, res; size_t out_len; int ret;
@@ -24,9 +25,11 @@ static u32 decompress_lzo(const u8 *cbuf, u32 clen, u8 *dbuf, u32 dlen) return -1;
tot_len = le32_to_cpu(get_unaligned((u32 *)cbuf)); + tot_in = 0; cbuf += LZO_LEN; clen -= LZO_LEN; tot_len -= LZO_LEN; + tot_in += LZO_LEN;
if (tot_len == 0 && dlen) return -1; @@ -36,6 +39,8 @@ static u32 decompress_lzo(const u8 *cbuf, u32 clen, u8 *dbuf, u32 dlen) res = 0;
while (tot_len > LZO_LEN) { + u32 rem_page; + in_len = le32_to_cpu(get_unaligned((u32 *)cbuf)); cbuf += LZO_LEN; clen -= LZO_LEN; @@ -44,6 +49,7 @@ static u32 decompress_lzo(const u8 *cbuf, u32 clen, u8 *dbuf, u32 dlen) return -1;
tot_len -= (LZO_LEN + in_len); + tot_in += (LZO_LEN + in_len);
out_len = dlen; ret = lzo1x_decompress_safe(cbuf, in_len, dbuf, &out_len); @@ -56,6 +62,18 @@ static u32 decompress_lzo(const u8 *cbuf, u32 clen, u8 *dbuf, u32 dlen) dlen -= out_len;
res += out_len; + + /* + * If the 4 bytes header does not fit to the rest of the page we + * have to move to next one, or we read some garbage. + */ + rem_page = PAGE_SIZE - (tot_in % PAGE_SIZE); + if (rem_page < LZO_LEN) { + cbuf += rem_page; + tot_in += rem_page; + clen -= rem_page; + tot_len -= rem_page; + } }
return res;
On Thu, 26 Mar 2020 13:35:56 +0800 Qu Wenruo wqu@suse.com wrote:
For certain btrfs files with compressed file extent, uboot will fail to load it:
btrfs_read_extent_reg: disk_bytenr=14229504 disk_len=73728 offset=0 nr_bytes=131 072 decompress_lzo: tot_len=70770 decompress_lzo: in_len=1389 decompress_lzo: in_len=2400 decompress_lzo: in_len=3002 decompress_lzo: in_len=1379 decompress_lzo: in_len=88539136 decompress_lzo: header error, in_len=88539136 clen=65534 tot_len=62580
NOTE: except the last line, all other lines are debug output.
Btrfs lzo compression uses its own format to record compressed size (segment header, LE32).
However to make decompression easier, we never put such segment header across page boundary.
In above case, the xxd dump of the lzo compressed data looks like this:
00001fe0: 4cdc 02fc 0bfd 02c0 dc02 0d13 0100 0001 L............... 00001ff0: 0000 0008 0300 0000 0000 0011 0000|0000 ................ 00002000: 4705 0000 0001 cc02 0000 0000 0000 1e01 G...............
'|' is the "expected" segment header start position.
But in that page, there are only 2 bytes left, can't contain the 4 bytes segment header.
So btrfs compression will skip that 2 bytes, put the segment header in next page directly.
Uboot doesn't have such check, and read the header with 2 bytes offset, result 0x05470000 (88539136), other than the expected result 0x00000547 (1351), resulting above error.
Follow the btrfs-progs restore implementation, by introducing tot_in to record total processed bytes (including headers), and do proper page boundary skip to fix it.
Please note that, current code base doesn't parse fs_info thus we can't grab sector size easily, so it uses PAGE_SIZE, and relying on fs open time check to exclude unsupported sector size.
Signed-off-by: Qu Wenruo wqu@suse.com Cc: Marek Behun marek.behun@nic.cz
fs/btrfs/compression.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c index 4ef44ce11485..b1884fc15ee0 100644 --- a/fs/btrfs/compression.c +++ b/fs/btrfs/compression.c @@ -9,6 +9,7 @@ #include <malloc.h> #include <linux/lzo.h> #include <linux/zstd.h> +#include <linux/compat.h> #include <u-boot/zlib.h> #include <asm/unaligned.h>
@@ -16,7 +17,7 @@ #define LZO_LEN 4 static u32 decompress_lzo(const u8 *cbuf, u32 clen, u8 *dbuf, u32 dlen) {
- u32 tot_len, in_len, res;
- u32 tot_len, tot_in, in_len, res; size_t out_len; int ret;
@@ -24,9 +25,11 @@ static u32 decompress_lzo(const u8 *cbuf, u32 clen, u8 *dbuf, u32 dlen) return -1;
tot_len = le32_to_cpu(get_unaligned((u32 *)cbuf));
tot_in = 0; cbuf += LZO_LEN; clen -= LZO_LEN; tot_len -= LZO_LEN;
tot_in += LZO_LEN;
if (tot_len == 0 && dlen) return -1;
@@ -36,6 +39,8 @@ static u32 decompress_lzo(const u8 *cbuf, u32 clen, u8 *dbuf, u32 dlen) res = 0;
while (tot_len > LZO_LEN) {
u32 rem_page;- in_len = le32_to_cpu(get_unaligned((u32 *)cbuf)); cbuf += LZO_LEN; clen -= LZO_LEN;
@@ -44,6 +49,7 @@ static u32 decompress_lzo(const u8 *cbuf, u32 clen, u8 *dbuf, u32 dlen) return -1;
tot_len -= (LZO_LEN + in_len);
tot_in += (LZO_LEN + in_len);out_len = dlen; ret = lzo1x_decompress_safe(cbuf, in_len, dbuf, &out_len);
@@ -56,6 +62,18 @@ static u32 decompress_lzo(const u8 *cbuf, u32 clen, u8 *dbuf, u32 dlen) dlen -= out_len;
res += out_len;
/** If the 4 bytes header does not fit to the rest of the page we* have to move to next one, or we read some garbage.*/rem_page = PAGE_SIZE - (tot_in % PAGE_SIZE);if (rem_page < LZO_LEN) {cbuf += rem_page;tot_in += rem_page;clen -= rem_page;tot_len -= rem_page;}}
return res;
Reviewed-by: Marek Behún marek.behun@nic.cz
On Thu, Mar 26, 2020 at 01:35:56PM +0800, Qu Wenruo wrote:
For certain btrfs files with compressed file extent, uboot will fail to load it:
btrfs_read_extent_reg: disk_bytenr=14229504 disk_len=73728 offset=0 nr_bytes=131 072 decompress_lzo: tot_len=70770 decompress_lzo: in_len=1389 decompress_lzo: in_len=2400 decompress_lzo: in_len=3002 decompress_lzo: in_len=1379 decompress_lzo: in_len=88539136 decompress_lzo: header error, in_len=88539136 clen=65534 tot_len=62580
NOTE: except the last line, all other lines are debug output.
Btrfs lzo compression uses its own format to record compressed size (segment header, LE32).
However to make decompression easier, we never put such segment header across page boundary.
In above case, the xxd dump of the lzo compressed data looks like this:
00001fe0: 4cdc 02fc 0bfd 02c0 dc02 0d13 0100 0001 L............... 00001ff0: 0000 0008 0300 0000 0000 0011 0000|0000 ................ 00002000: 4705 0000 0001 cc02 0000 0000 0000 1e01 G...............
'|' is the "expected" segment header start position.
But in that page, there are only 2 bytes left, can't contain the 4 bytes segment header.
So btrfs compression will skip that 2 bytes, put the segment header in next page directly.
Uboot doesn't have such check, and read the header with 2 bytes offset, result 0x05470000 (88539136), other than the expected result 0x00000547 (1351), resulting above error.
Follow the btrfs-progs restore implementation, by introducing tot_in to record total processed bytes (including headers), and do proper page boundary skip to fix it.
Please note that, current code base doesn't parse fs_info thus we can't grab sector size easily, so it uses PAGE_SIZE, and relying on fs open time check to exclude unsupported sector size.
Signed-off-by: Qu Wenruo wqu@suse.com Cc: Marek Behun marek.behun@nic.cz Reviewed-by: Marek Behún marek.behun@nic.cz
Applied to u-boot/master, thanks!
participants (3)
-
Marek Behun -
Qu Wenruo -
Tom Rini