Bug 202747 - sometime kernel crash when kzfree is called in fs/f2fs/xattr.c
Summary: sometime kernel crash when kzfree is called in fs/f2fs/xattr.c
Status: RESOLVED CODE_FIX
Alias: None
Product: File System
Classification: Unclassified
Component: f2fs (show other bugs)
Hardware: All Linux
: P1 high
Assignee: Default virtual assignee for f2fs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-03-04 13:30 UTC by Jiqun Li
Modified: 2019-03-16 08:09 UTC (History)
1 user (show)

See Also:
Kernel Version: f2fs-dev
Subsystem:
Regression: No
Bisected commit-id:


Attachments

Description Jiqun Li 2019-03-04 13:30:53 UTC
in file fs/f2fs/xattr.c

kzfree() is called  somewhere

example:

static int read_all_xattrs(struct inode *inode, struct page *ipage,
							void **base_addr)
{
        ......
	txattr_addr = f2fs_kzalloc(F2FS_I_SB(inode),
			inline_size + size + XATTR_PADDING_SIZE, GFP_NOFS);
        .....
	kzfree(txattr_addr);
	return err;
}

address is alloced by f2fs_kzalloc(), step into, it may use kmalloc() or kvmalloc(), 
accordingly the address should be freed by kfree() or kvfree(), but kzfree() aways use kfree(), then the kernel crashed when the address is alloced by kvmalloc().

I have changed kzfree to kvfree, kernel not crash any more, I not understand why set the memory to zero before free the address.

so I use the patch, kernel not crash also。

it diff diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h index 85d9508..c4b3d7b 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -2798,6 +2798,18 @@ static inline void *f2fs_kmalloc(struct f2fs_sb_info *sbi,
        return kvmalloc(size, flags);
}

+static inline void *f2fs_kzfree(const void *p) {
+       size_t ks;
+       void *mem = (void *)p;
+
+       if (unlikely(ZERO_OR_NULL_PTR(mem)))
+               return;
+       ks = ksize(mem);
+       memset(mem, 0, ks);
+       kvfree(mem);
+}
+
static inline void *kvzalloc(size_t size, gfp_t flags) {
        void *ret;
diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c index dedc91a..0152ed8 100644
--- a/fs/f2fs/xattr.c
+++ b/fs/f2fs/xattr.c
@@ -386,7 +386,7 @@ check:
        *base_addr = txattr_addr;
        return 0;
out:
-       kzfree(txattr_addr);
+       f2fs_kzfree(txattr_addr);
        return err;
}

@@ -429,7 +429,7 @@ static int read_all_xattrs(struct inode *inode, struct page *ipage,
        *base_addr = txattr_addr;
        return 0;
fail:
-       kzfree(txattr_addr);
+       f2fs_kzfree(txattr_addr);
        return err;
}

@@ -556,7 +556,7 @@ int f2fs_getxattr(struct inode *inode, int index, const char *name,
        }
        error = size;
out:
-       kzfree(base_addr);
+       f2fs_kzfree(base_addr);
        return error;
Comment 1 Chao Yu 2019-03-05 06:40:30 UTC
I didn't see many filesystems are using kzfree(), instead, there are many callers come from crypto module, I guess they use the buffer to store ciphertext or crypto key temporarily, so, before freeing those buffer it will be better to clean the data in buffer to avoid confidential data leak.

For f2fs, I think there is no such demand, and we just missed to change kzfree to kvfree in below commit:

5222595d093e ("f2fs: use kvmalloc, if kmalloc is failed")
Comment 2 Chao Yu 2019-03-16 08:09:22 UTC
The fixing patch has been merged, close this issue.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2a6a7e722e7a78d774ce02b847c5b183a3ff2672

Note You need to log in before you can comment on or make changes to this bug.