[PATCH 6.1/5.15/5.10/5.4] udf: fix null-ptr-deref if sb_getblk() fails

[PATCH 6.1/5.15/5.10/5.4] udf: fix null-ptr-deref if sb_getblk() fails

[Jakub Acs]

commit 32f123a3f342 ("udf: Fold udf_getblk() into udf_bread()"), fixes a
null-ptr-deref bug as a side effect. Backport the null-ptr-deref fixing
aspect of the aforementioned commit.

Closes: https://syzkaller.appspot.com/bug?extid=a38e34ca637c224f4a79
Signed-off-by: Jakub Acs <acsjakub@amazon.de>
---
 fs/udf/inode.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index d7d6ccd0af06..4f505a366da9 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -380,6 +380,10 @@ static struct buffer_head *udf_getblk(struct inode *inode, udf_pblk_t block,
 	*err = udf_get_block(inode, block, &dummy, create);
 	if (!*err && buffer_mapped(&dummy)) {
 		bh = sb_getblk(inode->i_sb, dummy.b_blocknr);
+		if (!bh) {
+			*err = -ENOMEM;
+			return NULL;
+		}
 		if (buffer_new(&dummy)) {
 			lock_buffer(bh);
 			memset(bh->b_data, 0x00, inode->i_sb->s_blocksize);

base-commit: e4d90d63d385228b1e0bcf31cc15539bbbc28f7f
-- 
2.40.1

Re: [PATCH 6.1/5.15/5.10/5.4] udf: fix null-ptr-deref if sb_getblk() fails

[Greg KH]

On Fri, Nov 29, 2024 at 10:58:46AM +0000, Jakub Acs wrote:
> commit 32f123a3f342 ("udf: Fold udf_getblk() into udf_bread()"), fixes a
> null-ptr-deref bug as a side effect. Backport the null-ptr-deref fixing
> aspect of the aforementioned commit.

Please backport the whole thing.

thanks,

greg k-h

[PATCH v2 6.1] udf: Fold udf_getblk() into udf_bread()

[Jakub Acs]

commit 32f123a3f34283f9c6446de87861696f0502b02e upstream.

udf_getblk() has a single call site. Fold it there.

Signed-off-by: Jan Kara <jack@suse.cz>

[acsjakub: backport-adjusting changes]
udf_getblk() has changed between 6.1 and the backported commit namely
in commit 541e047b14c8 ("udf: Use udf_map_block() in udf_getblk()")

Backport using the form of udf_getblk present in 6.1., that means use
udf_get_block() instead of udf_map_block() and use dummy in buffer_new()
and buffer_mapped().

Closes: https://syzkaller.appspot.com/bug?extid=a38e34ca637c224f4a79
Signed-off-by: Jakub Acs <acsjakub@amazon.de>
---
While doing the backport I have noticed potential side effect of the
upstream commit (present in the mainline):

If we take the if-branch of 'if (map.oflags & UDF_BLK_NEW)', we will
return the bh without the 'if (bh_read(bh, 0) >= 0)' check. Prior to
the folding, the check wouldn't be skipped, was this intentional by the
upstream commit?
---
 fs/udf/inode.c | 46 +++++++++++++++++++++-------------------------
 1 file changed, 21 insertions(+), 25 deletions(-)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index d7d6ccd0af06..626450101412 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -369,29 +369,6 @@ static int udf_get_block(struct inode *inode, sector_t block,
 	return err;
 }
 
-static struct buffer_head *udf_getblk(struct inode *inode, udf_pblk_t block,
-				      int create, int *err)
-{
-	struct buffer_head *bh;
-	struct buffer_head dummy;
-
-	dummy.b_state = 0;
-	dummy.b_blocknr = -1000;
-	*err = udf_get_block(inode, block, &dummy, create);
-	if (!*err && buffer_mapped(&dummy)) {
-		bh = sb_getblk(inode->i_sb, dummy.b_blocknr);
-		if (buffer_new(&dummy)) {
-			lock_buffer(bh);
-			memset(bh->b_data, 0x00, inode->i_sb->s_blocksize);
-			set_buffer_uptodate(bh);
-			unlock_buffer(bh);
-			mark_buffer_dirty_inode(bh, inode);
-		}
-		return bh;
-	}
-
-	return NULL;
-}
 
 /* Extend the file with new blocks totaling 'new_block_bytes',
  * return the number of extents added
@@ -1108,10 +1085,29 @@ struct buffer_head *udf_bread(struct inode *inode, udf_pblk_t block,
 			      int create, int *err)
 {
 	struct buffer_head *bh = NULL;
+	struct buffer_head dummy;
 
-	bh = udf_getblk(inode, block, create, err);
-	if (!bh)
+	dummy.b_state = 0;
+	dummy.b_blocknr = -1000;
+
+	*err = udf_get_block(inode, block, &dummy, create);
+	if (*err || !buffer_mapped(&dummy))
+		return NULL
+
+	bh = sb_getblk(inode->i_sb, dummy.b_blocknr);
+	if (!bh) {
+		*err = -ENOMEM;
 		return NULL;
+	}
+
+	if (buffer_new(&dummy)) {
+		lock_buffer(bh);
+		memset(bh->b_data, 0x00, inode->i_sb->s_blocksize);
+		set_buffer_uptodate(bh);
+		unlock_buffer(bh);
+		mark_buffer_dirty_inode(bh, inode);
+		return bh;
+	}
 
 	if (bh_read(bh, 0) >= 0)
 		return bh;

base-commit: e4d90d63d385228b1e0bcf31cc15539bbbc28f7f
-- 
2.40.1

Re: [PATCH v2 6.1] udf: Fold udf_getblk() into udf_bread()

[Jan Kara]

On Wed 04-12-24 09:32:26, Jakub Acs wrote:
> commit 32f123a3f34283f9c6446de87861696f0502b02e upstream.
> 
> udf_getblk() has a single call site. Fold it there.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> 
> [acsjakub: backport-adjusting changes]
> udf_getblk() has changed between 6.1 and the backported commit namely
> in commit 541e047b14c8 ("udf: Use udf_map_block() in udf_getblk()")
> 
> Backport using the form of udf_getblk present in 6.1., that means use
> udf_get_block() instead of udf_map_block() and use dummy in buffer_new()
> and buffer_mapped().
> 
> Closes: https://syzkaller.appspot.com/bug?extid=a38e34ca637c224f4a79
> Signed-off-by: Jakub Acs <acsjakub@amazon.de>
> ---
> While doing the backport I have noticed potential side effect of the
> upstream commit (present in the mainline):
> 
> If we take the if-branch of 'if (map.oflags & UDF_BLK_NEW)', we will
> return the bh without the 'if (bh_read(bh, 0) >= 0)' check. Prior to
> the folding, the check wouldn't be skipped, was this intentional by the
> upstream commit?

Absolutely. bh_read() is pointless if you fill in the buffer contents
yourself (as we do in the 'if (map.oflags & UDF_BLK_NEW)' branch).

								Honza

> ---
>  fs/udf/inode.c | 46 +++++++++++++++++++++-------------------------
>  1 file changed, 21 insertions(+), 25 deletions(-)
> 
> diff --git a/fs/udf/inode.c b/fs/udf/inode.c
> index d7d6ccd0af06..626450101412 100644
> --- a/fs/udf/inode.c
> +++ b/fs/udf/inode.c
> @@ -369,29 +369,6 @@ static int udf_get_block(struct inode *inode, sector_t block,
>  	return err;
>  }
>  
> -static struct buffer_head *udf_getblk(struct inode *inode, udf_pblk_t block,
> -				      int create, int *err)
> -{
> -	struct buffer_head *bh;
> -	struct buffer_head dummy;
> -
> -	dummy.b_state = 0;
> -	dummy.b_blocknr = -1000;
> -	*err = udf_get_block(inode, block, &dummy, create);
> -	if (!*err && buffer_mapped(&dummy)) {
> -		bh = sb_getblk(inode->i_sb, dummy.b_blocknr);
> -		if (buffer_new(&dummy)) {
> -			lock_buffer(bh);
> -			memset(bh->b_data, 0x00, inode->i_sb->s_blocksize);
> -			set_buffer_uptodate(bh);
> -			unlock_buffer(bh);
> -			mark_buffer_dirty_inode(bh, inode);
> -		}
> -		return bh;
> -	}
> -
> -	return NULL;
> -}
>  
>  /* Extend the file with new blocks totaling 'new_block_bytes',
>   * return the number of extents added
> @@ -1108,10 +1085,29 @@ struct buffer_head *udf_bread(struct inode *inode, udf_pblk_t block,
>  			      int create, int *err)
>  {
>  	struct buffer_head *bh = NULL;
> +	struct buffer_head dummy;
>  
> -	bh = udf_getblk(inode, block, create, err);
> -	if (!bh)
> +	dummy.b_state = 0;
> +	dummy.b_blocknr = -1000;
> +
> +	*err = udf_get_block(inode, block, &dummy, create);
> +	if (*err || !buffer_mapped(&dummy))
> +		return NULL
> +
> +	bh = sb_getblk(inode->i_sb, dummy.b_blocknr);
> +	if (!bh) {
> +		*err = -ENOMEM;
>  		return NULL;
> +	}
> +
> +	if (buffer_new(&dummy)) {
> +		lock_buffer(bh);
> +		memset(bh->b_data, 0x00, inode->i_sb->s_blocksize);
> +		set_buffer_uptodate(bh);
> +		unlock_buffer(bh);
> +		mark_buffer_dirty_inode(bh, inode);
> +		return bh;
> +	}
>  
>  	if (bh_read(bh, 0) >= 0)
>  		return bh;
> 
> base-commit: e4d90d63d385228b1e0bcf31cc15539bbbc28f7f
> -- 
> 2.40.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 6.1] udf: Fold udf_getblk() into udf_bread()

[Sasha Levin]

[ Sasha's backport helper bot ]

Hi,

The upstream commit SHA1 provided is correct: 32f123a3f34283f9c6446de87861696f0502b02e

WARNING: Author mismatch between patch and upstream commit:
Backport author: Jakub Acs <acsjakub@amazon.com>
Commit author: Jan Kara <jack@suse.cz>


Status in newer kernel trees:
6.12.y | Present (exact SHA1)
6.11.y | Present (exact SHA1)
6.6.y | Present (exact SHA1)
6.1.y | Not found

Note: The patch differs from the upstream commit:
---
1:  32f123a3f3428 < -:  ------------- udf: Fold udf_getblk() into udf_bread()
-:  ------------- > 1:  9087b1856465c udf: Fold udf_getblk() into udf_bread()
---

Results of testing on various branches:

| Branch                    | Patch Apply | Build Test |
|---------------------------|-------------|------------|
| stable/linux-6.1.y        |  Success    |  Failed    |

Build Errors:
Build error for stable/linux-6.1.y:
    fs/udf/inode.c: In function 'udf_bread':
    fs/udf/inode.c:1097:9: error: expected ';' before 'bh'
     1097 |         bh = sb_getblk(inode->i_sb, dummy.b_blocknr);
          |         ^~
    make[3]: *** [scripts/Makefile.build:250: fs/udf/inode.o] Error 1
    make[3]: Target 'fs/udf/' not remade because of errors.
    make[2]: *** [scripts/Makefile.build:503: fs/udf] Error 2
    make[2]: Target 'fs/' not remade because of errors.
    make[1]: *** [scripts/Makefile.build:503: fs] Error 2
    make[1]: Target './' not remade because of errors.
    make: *** [Makefile:2009: .] Error 2
    make: Target '__all' not remade because of errors.

Re: [PATCH v2 6.1] udf: Fold udf_getblk() into udf_bread()

[Sasha Levin]

[ Sasha's backport helper bot ]

Hi,

The upstream commit SHA1 provided is correct: 32f123a3f34283f9c6446de87861696f0502b02e

WARNING: Author mismatch between patch and upstream commit:
Backport author: Jakub Acs <acsjakub@amazon.com>
Commit author: Jan Kara <jack@suse.cz>


Status in newer kernel trees:
6.12.y | Present (exact SHA1)
6.11.y | Present (exact SHA1)
6.6.y | Present (exact SHA1)
6.1.y | Not found

Note: The patch differs from the upstream commit:
---
1:  32f123a3f3428 < -:  ------------- udf: Fold udf_getblk() into udf_bread()
-:  ------------- > 1:  9087b1856465c udf: Fold udf_getblk() into udf_bread()
---

Results of testing on various branches:

| Branch                    | Patch Apply | Build Test |
|---------------------------|-------------|------------|
| stable/linux-6.1.y        |  Success    |  Failed    |

Build Errors:
Build error for stable/linux-6.1.y:
    fs/udf/inode.c: In function 'udf_bread':
    fs/udf/inode.c:1097:9: error: expected ';' before 'bh'
     1097 |         bh = sb_getblk(inode->i_sb, dummy.b_blocknr);
          |         ^~
    make[3]: *** [scripts/Makefile.build:250: fs/udf/inode.o] Error 1
    make[3]: Target 'fs/udf/' not remade because of errors.
    make[2]: *** [scripts/Makefile.build:503: fs/udf] Error 2
    make[2]: Target 'fs/' not remade because of errors.
    make[1]: *** [scripts/Makefile.build:503: fs] Error 2
    make[1]: Target './' not remade because of errors.
    make: *** [Makefile:2009: .] Error 2
    make: Target '__all' not remade because of errors.

[PATCH v3 6.1] udf: Fold udf_getblk() into udf_bread()

[Jakub Acs]

commit 32f123a3f34283f9c6446de87861696f0502b02e upstream.

udf_getblk() has a single call site. Fold it there.

Signed-off-by: Jan Kara <jack@suse.cz>

[acsjakub: backport-adjusting changes]
udf_getblk() has changed between 6.1 and the backported commit, namely
in commit 541e047b14c8 ("udf: Use udf_map_block() in udf_getblk()")

Backport using the form of udf_getblk present in 6.1., that means use
udf_get_block() instead of udf_map_block() and use dummy in buffer_new()
and buffer_mapped().

Closes: https://syzkaller.appspot.com/bug?extid=a38e34ca637c224f4a79
Signed-off-by: Jakub Acs <acsjakub@amazon.de>
---
v3: fix the missing ';', sorry about that

 fs/udf/inode.c | 46 +++++++++++++++++++++-------------------------
 1 file changed, 21 insertions(+), 25 deletions(-)

diff --git a/fs/udf/inode.c b/fs/udf/inode.c
index d7d6ccd0af06..e2ac428f3809 100644
--- a/fs/udf/inode.c
+++ b/fs/udf/inode.c
@@ -369,29 +369,6 @@ static int udf_get_block(struct inode *inode, sector_t block,
 	return err;
 }
 
-static struct buffer_head *udf_getblk(struct inode *inode, udf_pblk_t block,
-				      int create, int *err)
-{
-	struct buffer_head *bh;
-	struct buffer_head dummy;
-
-	dummy.b_state = 0;
-	dummy.b_blocknr = -1000;
-	*err = udf_get_block(inode, block, &dummy, create);
-	if (!*err && buffer_mapped(&dummy)) {
-		bh = sb_getblk(inode->i_sb, dummy.b_blocknr);
-		if (buffer_new(&dummy)) {
-			lock_buffer(bh);
-			memset(bh->b_data, 0x00, inode->i_sb->s_blocksize);
-			set_buffer_uptodate(bh);
-			unlock_buffer(bh);
-			mark_buffer_dirty_inode(bh, inode);
-		}
-		return bh;
-	}
-
-	return NULL;
-}
 
 /* Extend the file with new blocks totaling 'new_block_bytes',
  * return the number of extents added
@@ -1108,11 +1085,30 @@ struct buffer_head *udf_bread(struct inode *inode, udf_pblk_t block,
 			      int create, int *err)
 {
 	struct buffer_head *bh = NULL;
+	struct buffer_head dummy;
 
-	bh = udf_getblk(inode, block, create, err);
-	if (!bh)
+	dummy.b_state = 0;
+	dummy.b_blocknr = -1000;
+
+	*err = udf_get_block(inode, block, &dummy, create);
+	if (*err || !buffer_mapped(&dummy))
 		return NULL;
 
+	bh = sb_getblk(inode->i_sb, dummy.b_blocknr);
+	if (!bh) {
+		*err = -ENOMEM;
+		return NULL;
+	}
+
+	if (buffer_new(&dummy)) {
+		lock_buffer(bh);
+		memset(bh->b_data, 0x00, inode->i_sb->s_blocksize);
+		set_buffer_uptodate(bh);
+		unlock_buffer(bh);
+		mark_buffer_dirty_inode(bh, inode);
+		return bh;
+	}
+
 	if (bh_read(bh, 0) >= 0)
 		return bh;
 

base-commit: e4d90d63d385228b1e0bcf31cc15539bbbc28f7f
-- 
2.40.1

Re: [PATCH v3 6.1] udf: Fold udf_getblk() into udf_bread()

[Sasha Levin]

[ Sasha's backport helper bot ]

Hi,

The upstream commit SHA1 provided is correct: 32f123a3f34283f9c6446de87861696f0502b02e

WARNING: Author mismatch between patch and upstream commit:
Backport author: Jakub Acs <acsjakub@amazon.com>
Commit author: Jan Kara <jack@suse.cz>


Status in newer kernel trees:
6.12.y | Present (exact SHA1)
6.11.y | Present (exact SHA1)
6.6.y | Present (exact SHA1)
6.1.y | Not found

Note: The patch differs from the upstream commit:
---
1:  32f123a3f3428 ! 1:  8293ff38b97ef udf: Fold udf_getblk() into udf_bread()
    @@
      ## Metadata ##
    -Author: Jan Kara <jack@suse.cz>
    +Author: Jakub Acs <acsjakub@amazon.com>
     
      ## Commit message ##
         udf: Fold udf_getblk() into udf_bread()
     
    +    commit 32f123a3f34283f9c6446de87861696f0502b02e upstream.
    +
         udf_getblk() has a single call site. Fold it there.
     
         Signed-off-by: Jan Kara <jack@suse.cz>
     
    +    [acsjakub: backport-adjusting changes]
    +    udf_getblk() has changed between 6.1 and the backported commit, namely
    +    in commit 541e047b14c8 ("udf: Use udf_map_block() in udf_getblk()")
    +
    +    Backport using the form of udf_getblk present in 6.1., that means use
    +    udf_get_block() instead of udf_map_block() and use dummy in buffer_new()
    +    and buffer_mapped().
    +
    +    Closes: https://syzkaller.appspot.com/bug?extid=a38e34ca637c224f4a79
    +    Signed-off-by: Jakub Acs <acsjakub@amazon.de>
    +
      ## fs/udf/inode.c ##
     @@ fs/udf/inode.c: static int udf_get_block(struct inode *inode, sector_t block,
    - 	return 0;
    + 	return err;
      }
      
     -static struct buffer_head *udf_getblk(struct inode *inode, udf_pblk_t block,
     -				      int create, int *err)
     -{
     -	struct buffer_head *bh;
    --	struct udf_map_rq map = {
    --		.lblk = block,
    --		.iflags = UDF_MAP_NOPREALLOC | (create ? UDF_MAP_CREATE : 0),
    --	};
    +-	struct buffer_head dummy;
     -
    --	*err = udf_map_block(inode, &map);
    --	if (!*err && map.oflags & UDF_BLK_MAPPED) {
    --		bh = sb_getblk(inode->i_sb, map.pblk);
    --		if (map.oflags & UDF_BLK_NEW) {
    +-	dummy.b_state = 0;
    +-	dummy.b_blocknr = -1000;
    +-	*err = udf_get_block(inode, block, &dummy, create);
    +-	if (!*err && buffer_mapped(&dummy)) {
    +-		bh = sb_getblk(inode->i_sb, dummy.b_blocknr);
    +-		if (buffer_new(&dummy)) {
     -			lock_buffer(bh);
     -			memset(bh->b_data, 0x00, inode->i_sb->s_blocksize);
     -			set_buffer_uptodate(bh);
    @@ fs/udf/inode.c: static int udf_get_block(struct inode *inode, sector_t block,
     -
     -	return NULL;
     -}
    --
    + 
      /* Extend the file with new blocks totaling 'new_block_bytes',
       * return the number of extents added
    -  */
     @@ fs/udf/inode.c: struct buffer_head *udf_bread(struct inode *inode, udf_pblk_t block,
      			      int create, int *err)
      {
      	struct buffer_head *bh = NULL;
    -+	struct udf_map_rq map = {
    -+		.lblk = block,
    -+		.iflags = UDF_MAP_NOPREALLOC | (create ? UDF_MAP_CREATE : 0),
    -+	};
    ++	struct buffer_head dummy;
      
     -	bh = udf_getblk(inode, block, create, err);
     -	if (!bh)
    -+	*err = udf_map_block(inode, &map);
    -+	if (*err || !(map.oflags & UDF_BLK_MAPPED))
    ++	dummy.b_state = 0;
    ++	dummy.b_blocknr = -1000;
    ++
    ++	*err = udf_get_block(inode, block, &dummy, create);
    ++	if (*err || !buffer_mapped(&dummy))
      		return NULL;
      
    -+	bh = sb_getblk(inode->i_sb, map.pblk);
    ++	bh = sb_getblk(inode->i_sb, dummy.b_blocknr);
     +	if (!bh) {
     +		*err = -ENOMEM;
     +		return NULL;
     +	}
    -+	if (map.oflags & UDF_BLK_NEW) {
    ++
    ++	if (buffer_new(&dummy)) {
     +		lock_buffer(bh);
     +		memset(bh->b_data, 0x00, inode->i_sb->s_blocksize);
     +		set_buffer_uptodate(bh);
---

Results of testing on various branches:

| Branch                    | Patch Apply | Build Test |
|---------------------------|-------------|------------|
| stable/linux-6.1.y        |  Success    |  Success   |

Re: [PATCH 6.1/5.15/5.10/5.4] udf: fix null-ptr-deref if sb_getblk() fails

[Sasha Levin]

[ Sasha's backport helper bot ]

Hi,

No upstream commit was identified. Using temporary commit for testing.

Results of testing on various branches:

| Branch                    | Patch Apply | Build Test |
|---------------------------|-------------|------------|
| stable/linux-6.1.y        |  Success    |  Success   |
| stable/linux-5.15.y       |  Success    |  Success   |
| stable/linux-5.10.y       |  Success    |  Success   |
| stable/linux-5.4.y        |  Success    |  Success   |