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
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
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
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;- }
- 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
[ 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.
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
[ 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 |
[ 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 |
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Jakub Acs -
Jan Kara -
Sasha Levin