[PATCH] fs: btrfs: fix reading when length specified

[PATCH] fs: btrfs: fix reading when length specified

[Sam Edwards]

The btrfs read function limits the read length to ensure that it
and the read offset do not together exceed the size of the file.
However, this size was only being queried if the read length was
passed a value of zero (meaning "whole file"), and the size is
defaulted to 0 otherwise. This means the clamp will just zero out
the length if one is specified, preventing reading of the file.

Fix this by checking the file size unconditionally, and unifying
the default length and clamping logic as a single range check instead.

This bug was discovered when trying to boot Linux with initrd= via
'bootefi' from a btrfs partition. The EFI stub entered an infinite
loop of zero-length reads while trying to read the initrd, and the
boot process stalled indefinitely.

Signed-off-by: Sam Edwards <CFSworks@gmail.com>
---
 fs/btrfs/btrfs.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/btrfs.c b/fs/btrfs/btrfs.c
index 4cdbbbe3d0..1149a3b200 100644
--- a/fs/btrfs/btrfs.c
+++ b/fs/btrfs/btrfs.c
@@ -228,7 +228,7 @@ int btrfs_read(const char *file, void *buf, loff_t offset, loff_t len,
 {
 	struct btrfs_fs_info *fs_info = current_fs_info;
 	struct btrfs_root *root;
-	loff_t real_size = 0;
+	loff_t real_size;
 	u64 ino;
 	u8 type;
 	int ret;
@@ -246,16 +246,13 @@ int btrfs_read(const char *file, void *buf, loff_t offset, loff_t len,
 		return -EINVAL;
 	}
 
-	if (!len) {
-		ret = btrfs_size(file, &real_size);
-		if (ret < 0) {
-			error("Failed to get inode size: %s", file);
-			return ret;
-		}
-		len = real_size;
+	ret = btrfs_size(file, &real_size);
+	if (ret < 0) {
+		error("Failed to get inode size: %s", file);
+		return ret;
 	}
 
-	if (len > real_size - offset)
+	if (!len || len > real_size - offset)
 		len = real_size - offset;
 
 	ret = btrfs_file_read(root, ino, offset, len, buf);
-- 
2.41.0

Re: [PATCH] fs: btrfs: fix reading when length specified

[Qu Wenruo]

On 2023/11/12 01:49, Sam Edwards wrote:
> The btrfs read function limits the read length to ensure that it
> and the read offset do not together exceed the size of the file.
> However, this size was only being queried if the read length was
> passed a value of zero (meaning "whole file"), and the size is
> defaulted to 0 otherwise. This means the clamp will just zero out
> the length if one is specified, preventing reading of the file.
>
> Fix this by checking the file size unconditionally, and unifying
> the default length and clamping logic as a single range check instead.
>
> This bug was discovered when trying to boot Linux with initrd= via
> 'bootefi' from a btrfs partition. The EFI stub entered an infinite
> loop of zero-length reads while trying to read the initrd, and the
> boot process stalled indefinitely.

I'm not sure why this happend for the EFI environment.

Doesn't the EFI runtime should also try to read the whole file? Or that
EFI environment has specified the length to read instead?

>
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>

Anyway, the fix looks good to me.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>   fs/btrfs/btrfs.c | 15 ++++++---------
>   1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/fs/btrfs/btrfs.c b/fs/btrfs/btrfs.c
> index 4cdbbbe3d0..1149a3b200 100644
> --- a/fs/btrfs/btrfs.c
> +++ b/fs/btrfs/btrfs.c
> @@ -228,7 +228,7 @@ int btrfs_read(const char *file, void *buf, loff_t offset, loff_t len,
>   {
>   	struct btrfs_fs_info *fs_info = current_fs_info;
>   	struct btrfs_root *root;
> -	loff_t real_size = 0;
> +	loff_t real_size;
>   	u64 ino;
>   	u8 type;
>   	int ret;
> @@ -246,16 +246,13 @@ int btrfs_read(const char *file, void *buf, loff_t offset, loff_t len,
>   		return -EINVAL;
>   	}
>
> -	if (!len) {
> -		ret = btrfs_size(file, &real_size);
> -		if (ret < 0) {
> -			error("Failed to get inode size: %s", file);
> -			return ret;
> -		}
> -		len = real_size;
> +	ret = btrfs_size(file, &real_size);
> +	if (ret < 0) {
> +		error("Failed to get inode size: %s", file);
> +		return ret;
>   	}
>
> -	if (len > real_size - offset)
> +	if (!len || len > real_size - offset)
>   		len = real_size - offset;
>
>   	ret = btrfs_file_read(root, ino, offset, len, buf);

Re: [PATCH] fs: btrfs: fix reading when length specified

[Sam Edwards]

On 11/15/23 21:43, Qu Wenruo wrote:
> 
> I'm not sure why this happend for the EFI environment.
> 
> Doesn't the EFI runtime should also try to read the whole file? Or that
> EFI environment has specified the length to read instead?
> 

Hi Qu,

The Linux EFISTUB file loading routine (handle_cmdline_files) reads 
files in chunks, and not (necessarily) the whole file in one go. On x86 
platforms, the chunk size is 1MB, and the comments explain that this is 
to work around firmware implementations that have problems with large 
reads. On non-x86 platforms, the chunk size is ULONG_MAX -- which in 
practice means it reads the whole file in one exact-filesize chunk.

So, to answer your questions: "either/both depending on platform."

(The bug in the U-Boot implementation doubtlessly affects more than just 
EFI; I only happened to discover it while trying to use EFI.)

>>
>> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> 
> Anyway, the fix looks good to me.
> 
> Reviewed-by: Qu Wenruo <wqu@suse.com>

Thank you greatly for the review!

> 
> Thanks,
> Qu

Likewise,
Sam

Re: [PATCH] fs: btrfs: fix reading when length specified

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1011 bytes --]

On Sat, Nov 11, 2023 at 08:19:04AM -0700, Sam Edwards wrote:

> The btrfs read function limits the read length to ensure that it
> and the read offset do not together exceed the size of the file.
> However, this size was only being queried if the read length was
> passed a value of zero (meaning "whole file"), and the size is
> defaulted to 0 otherwise. This means the clamp will just zero out
> the length if one is specified, preventing reading of the file.
> 
> Fix this by checking the file size unconditionally, and unifying
> the default length and clamping logic as a single range check instead.
> 
> This bug was discovered when trying to boot Linux with initrd= via
> 'bootefi' from a btrfs partition. The EFI stub entered an infinite
> loop of zero-length reads while trying to read the initrd, and the
> boot process stalled indefinitely.
> 
> Signed-off-by: Sam Edwards <CFSworks@gmail.com>
> Reviewed-by: Qu Wenruo <wqu@suse.com>

Applied to u-boot/next, thanks!

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]