Hello Jan,
Syzkaller reports the following problems [1].
null-ptr-deref is fixed by the following patch.
About out-of-bounds write: I suppose it is due to the similar issue which
caused null-ptr-deref bug - udf_find_entry() return value is not checked
and we can end up with an incorrect ocfi and ofibh structs. Actually,
padlen in udf_write_fi() becomes less than zero because the values it is
consisted of are invalid, then padlen is passed to memcpy and here is the
bug. Should I also add this to the patch description text (I'm not sure
about it as that is just a consequence of null-ptr-deref bug)?
With the applied patch reproducers on my machine do not trigger
null-ptr-deref and out-of-bounds anymore.
The only thing is that: does udf_rename() need to immediately return with
an error if udf_find_entry() fails here? If ofi is an error pointer
(especially -ENOMEM), udf_rename should return an error. But if ofi is
NULL, the entry has probably already been deleted, and we should just skip
udf_delete_entry() call.
[1] https://syzkaller.appspot.com/bug?id=ddfcda66fe98b595b0fe11d9dd64eb532478f04b
Fedor
udf_find_entry() can return NULL or an error pointer if it fails. So we
should check its return value to avoid NULL pointer dereferencing in
udf_write_fi() (which is called from udf_delete_entry()). Also, if
udf_find_entry() returns an error pointer, it is possible that ofibh and
ocfi structs hold invalid values which can cause additional problems in
udf_write_fi().
If udf_find_entry() returns an error pointer, udf_rename() should return
with an error code. If udf_find_entry() returns NULL, ofi has probably
already been deleted.
Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
Fixes: 231473f6ddce ("udf: Return error from udf_find_entry()")
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: [email protected]
Signed-off-by: Fedor Pchelkin <[email protected]>
Signed-off-by: Alexey Khoroshilov <[email protected]>
---
fs/udf/namei.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/fs/udf/namei.c b/fs/udf/namei.c
index 7c95c549dd64..6b058c6ebf93 100644
--- a/fs/udf/namei.c
+++ b/fs/udf/namei.c
@@ -1170,7 +1170,12 @@ static int udf_rename(struct user_namespace *mnt_userns, struct inode *old_dir,
/* The old fid may have moved - find it again */
ofi = udf_find_entry(old_dir, &old_dentry->d_name, &ofibh, &ocfi);
- udf_delete_entry(old_dir, ofi, &ofibh, &ocfi);
+ if (ofi && IS_ERR(ofi)) {
+ retval = PTR_ERR(ofi);
+ goto end_rename;
+ } else if (ofi) {
+ udf_delete_entry(old_dir, ofi, &ofibh, &ocfi);
+ }
if (new_inode) {
new_inode->i_ctime = current_time(new_inode);
--
2.34.1
On Sat 07-01-23 22:50:16, Fedor Pchelkin wrote:
> udf_find_entry() can return NULL or an error pointer if it fails. So we
> should check its return value to avoid NULL pointer dereferencing in
> udf_write_fi() (which is called from udf_delete_entry()). Also, if
> udf_find_entry() returns an error pointer, it is possible that ofibh and
> ocfi structs hold invalid values which can cause additional problems in
> udf_write_fi().
>
> If udf_find_entry() returns an error pointer, udf_rename() should return
> with an error code. If udf_find_entry() returns NULL, ofi has probably
> already been deleted.
>
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
>
> Fixes: 231473f6ddce ("udf: Return error from udf_find_entry()")
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: [email protected]
> Signed-off-by: Fedor Pchelkin <[email protected]>
> Signed-off-by: Alexey Khoroshilov <[email protected]>
Thanks for the patch but I have already queued in my tree [1] rewrite of
UDF directory handling code that addresses multiple issues syzbot found in
directory handling and as far as I'm looking into the new code, this one
should be fixed as well.
[1] git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git for_next
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR