diff mbox series

fhandle: add __GFP_ZERO flag for kmalloc in function do_sys_name_to_handle

Message ID 20220322021326.20162-1-tcs.kernel@gmail.com (mailing list archive)
State New, archived
Headers show
Series fhandle: add __GFP_ZERO flag for kmalloc in function do_sys_name_to_handle | expand

Commit Message

Haimin Zhang March 22, 2022, 2:13 a.m. UTC 
From: Haimin Zhang <tcs_kernel@tencent.com>

Add __GFP_ZERO flag for kmalloc in function do_sys_name_to_handle to
initialize the buffer of a file_handle variable.

Reported-by: TCS Robot <tcs_robot@tencent.com>
Signed-off-by: Haimin Zhang <tcs_kernel@tencent.com>
---
This can cause a two-bytes-size kernel-info-leak problem.
1. do_sys_name_to_handle calls kmalloc to allocate the buffer of a file_handle variable, but doesn't zero it properly.
```
kmalloc build/../include/linux/slab.h:586 [inline]
 do_sys_name_to_handle build/../fs/fhandle.c:40 [inline]
 __do_sys_name_to_handle_at build/../fs/fhandle.c:109 [inline]
 __se_sys_name_to_handle_at+0x470/0x990 build/../fs/fhandle.c:93
 __x64_sys_name_to_handle_at+0x15d/0x1b0 build/../fs/fhandle.c:93
 do_syscall_x64 build/../arch/x86/entry/common.c:51 [inline]
 do_syscall_64+0x54/0xd0 build/../arch/x86/entry/common.c:82
 entry_SYSCALL_64_after_hwframe+0x44/0xae
```
2. do_sys_name_to_handle calls exportfs_encode_fh to fill the content of the variable.
3. If the inode is a fat node, exportfs_encode_fh will call fat_encode_fh_nostale to further process.
```
#0  fat_encode_fh_nostale (inode=inode@entry=0xffff88815d9286a8, fh=fh@entry=0xffff88815ce7f008, lenp=lenp@entry=0xffff88814f5c3e4c,
    parent=parent@entry=0x0 <fixed_percpu_data>) at ../fs/fat/nfs.c:102
#1  0xffffffff8375cbe1 in exportfs_encode_inode_fh (inode=0xffff88815d9286a8, fid=0xffff88815ce7f008, max_len=0xffff88814f5c3e4c,
    parent=0x0 <fixed_percpu_data>) at ../fs/exportfs/expfs.c:391
#2  exportfs_encode_fh (dentry=0x0 <fixed_percpu_data>, dentry@entry=0xffff8881255cf480, fid=fid@entry=0xffff88815ce7f008,
    max_len=max_len@entry=0xffff88814f5c3e4c, connectable=connectable@entry=0x0) at ../fs/exportfs/expfs.c:413
#3  0xffffffff82a88c9b in do_sys_name_to_handle (path=0xffff88814f5c3e38, ufh=0x20000500, mnt_id=0x20000540) at ../fs/fhandle.c:49
#4  __do_sys_name_to_handle_at (dfd=<optimized out>, name=0x0 <fixed_percpu_data>, flag=<optimized out>, handle=<optimized out>, mnt_id=<optimized out>)
    at ../fs/fhandle.c:109
#5  __se_sys_name_to_handle_at (dfd=0xffff88815ce7f000, dfd@entry=0xffffff9c, name=0x0, name@entry=0x200004c0, handle=handle@entry=0x20000500,
    mnt_id=mnt_id@entry=0x20000540, flag=flag@entry=0x1000) at ../fs/fhandle.c:93
#6  0xffffffff82a8870d in __x64_sys_name_to_handle_at (regs=<optimized out>) at ../fs/fhandle.c:93
#7  0xffffffff8fb1c264 in do_syscall_x64 (regs=0xffff88814f5c3f58, nr=0x12f) at ../arch/x86/entry/common.c:51
#8  do_syscall_64 (regs=0xffff88814f5c3f58, nr=0x12f) at ../arch/x86/entry/common.c:82
#9  0xffffffff8fc00068 in entry_SYSCALL_64 () at ../net/unix/af_unix.c:3364
#10 0x0000000000000000 in ?? ()
```
4. If the argument parent is NULL, the lenp will be 3, it means that fat_encode_fh_nostale modifies 12 bytes of the buffer. But actually it just modifies 10 bytes.
```
struct fat_fid {
	u32 i_gen;
	u32 i_pos_low;
	u16 i_pos_hi;
	// The following fields will be assigned on if parent isn't NULL, 
	u16 parent_i_pos_hi;
	u32 parent_i_pos_low;
	u32 parent_i_gen;
};
```
5. When it returns to do_sys_name_to_handle, the whole 12 bytes will be copied to user space.
```
 copy_to_user build/../include/linux/uaccess.h:209 [inline]
 do_sys_name_to_handle build/../fs/fhandle.c:73 [inline]
 __do_sys_name_to_handle_at build/../fs/fhandle.c:109 [inline]
 __se_sys_name_to_handle_at+0x86b/0x990 build/../fs/fhandle.c:93
 __x64_sys_name_to_handle_at+0x15d/0x1b0 build/../fs/fhandle.c:93
 do_syscall_x64 build/../arch/x86/entry/common.c:51 [inline]
 do_syscall_64+0x54/0xd0 build/../arch/x86/entry/common.c:82
 entry_SYSCALL_64_after_hwframe+0x44/0xae
```
6. The following is debug information:
Bytes 18-19 of 20 are uninitialized
Memory access of size 20 starts at ffff88815e77a1e0
Data copied to user address 0000000020000500

 fs/fhandle.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Al Viro March 22, 2022, 2:41 a.m. UTC | #1 
On Tue, Mar 22, 2022 at 10:13:26AM +0800, Haimin Zhang wrote:
> From: Haimin Zhang <tcs_kernel@tencent.com>
> 
> Add __GFP_ZERO flag for kmalloc in function do_sys_name_to_handle to
> initialize the buffer of a file_handle variable.
> 
> Reported-by: TCS Robot <tcs_robot@tencent.com>
> Signed-off-by: Haimin Zhang <tcs_kernel@tencent.com>

That's called kzalloc()...  Care to resend?
diff mbox series

Patch

diff --git a/fs/fhandle.c b/fs/fhandle.c
index 6630c69c23a2..be6b9ed12dfd 100644
--- a/fs/fhandle.c
+++ b/fs/fhandle.c
@@ -38,7 +38,7 @@  static long do_sys_name_to_handle(struct path *path,
 		return -EINVAL;
 
 	handle = kmalloc(sizeof(struct file_handle) + f_handle.handle_bytes,
-			 GFP_KERNEL);
+			 GFP_KERNEL | __GFP_ZERO);
 	if (!handle)
 		return -ENOMEM;