* Re: ksmbd: fix lookup on idmapped mounts
@ 2021-08-19 13:11 Colin Ian King
2021-08-19 14:15 ` Christian Brauner
0 siblings, 1 reply; 2+ messages in thread
From: Colin Ian King @ 2021-08-19 13:11 UTC (permalink / raw)
To: Christian Brauner
Cc: Steve French, Christoph Hellwig, Namjae Jeon, Hyunchul Lee,
Sergey Senozhatsky, David Sterba, linux-cifs
Hi,
Static analysis on linux-next with Coverity has detected a potential
issue in fs/ksmbd/smb2pdu.c with the following commit:
commit 4b499755e1024f97e75411920a404b357af6e153
Author: Christian Brauner <christian.brauner@ubuntu.com>
Date: Mon Aug 16 13:56:05 2021 +0200
ksmbd: fix lookup on idmapped mounts
The analysis is as follows:
5626 static int set_rename_info(struct ksmbd_work *work, struct
ksmbd_file *fp,
5627 char *buf)
5628 {
1. var_decl: Declaring variable user_ns without initializer.
5629 struct user_namespace *user_ns;
5630 struct ksmbd_file *parent_fp;
5631 struct dentry *parent;
5632 struct dentry *dentry = fp->filp->f_path.dentry;
5633 int ret;
5634
2. Condition !(fp->daccess & 65536U /* (__le32)(__u32)65536 */),
taking false branch.
5635 if (!(fp->daccess & FILE_DELETE_LE)) {
5636 pr_err("no right to delete : 0x%x\n", fp->daccess);
5637 return -EACCES;
5638 }
5639
3. Condition ksmbd_stream_fd(fp), taking true branch.
5640 if (ksmbd_stream_fd(fp))
4. Jumping to label next.
5641 goto next;
5642
5643 user_ns = file_mnt_user_ns(fp->filp);
5644 parent = dget_parent(dentry);
5645 ret = ksmbd_vfs_lock_parent(user_ns, parent, dentry);
5646 if (ret) {
5647 dput(parent);
5648 return ret;
5649 }
5650
5651 parent_fp = ksmbd_lookup_fd_inode(d_inode(parent));
5652 inode_unlock(d_inode(parent));
5653 dput(parent);
5654
5655 if (parent_fp) {
5656 if (parent_fp->daccess & FILE_DELETE_LE) {
5657 pr_err("parent dir is opened with delete
access\n");
5658 return -ESHARE;
5659 }
5660 }
5661 next:
Uninitialized pointer read (UNINIT)
5. uninit_use_in_call: Using uninitialized value user_ns when
calling smb2_rename.
5662 return smb2_rename(work, fp, user_ns,
5663 (struct smb2_file_rename_info *)buf,
5664 work->sess->conn->local_nls);
5665 }
The pointer user_ns is not initialized and the goto on line 5641 will
cause this uninitialized pointer value to be passed to smb2_rename.
I suspect user_ns = file_mnt_user_ns(fp->filp) should be moved to line
5639 as a fix, but I'm concerned that this is not a suitable fix.
Colin
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: ksmbd: fix lookup on idmapped mounts
2021-08-19 13:11 ksmbd: fix lookup on idmapped mounts Colin Ian King
@ 2021-08-19 14:15 ` Christian Brauner
0 siblings, 0 replies; 2+ messages in thread
From: Christian Brauner @ 2021-08-19 14:15 UTC (permalink / raw)
To: Colin Ian King
Cc: Steve French, Christoph Hellwig, Namjae Jeon, Hyunchul Lee,
Sergey Senozhatsky, David Sterba, linux-cifs
On Thu, Aug 19, 2021 at 02:11:43PM +0100, Colin Ian King wrote:
> Hi,
>
> Static analysis on linux-next with Coverity has detected a potential
> issue in fs/ksmbd/smb2pdu.c with the following commit:
>
> commit 4b499755e1024f97e75411920a404b357af6e153
> Author: Christian Brauner <christian.brauner@ubuntu.com>
> Date: Mon Aug 16 13:56:05 2021 +0200
>
> ksmbd: fix lookup on idmapped mounts
>
> The analysis is as follows:
>
> 5626 static int set_rename_info(struct ksmbd_work *work, struct
> ksmbd_file *fp,
> 5627 char *buf)
> 5628 {
>
> 1. var_decl: Declaring variable user_ns without initializer.
>
> 5629 struct user_namespace *user_ns;
> 5630 struct ksmbd_file *parent_fp;
> 5631 struct dentry *parent;
> 5632 struct dentry *dentry = fp->filp->f_path.dentry;
> 5633 int ret;
> 5634
>
> 2. Condition !(fp->daccess & 65536U /* (__le32)(__u32)65536 */),
> taking false branch.
>
> 5635 if (!(fp->daccess & FILE_DELETE_LE)) {
> 5636 pr_err("no right to delete : 0x%x\n", fp->daccess);
> 5637 return -EACCES;
> 5638 }
> 5639
>
> 3. Condition ksmbd_stream_fd(fp), taking true branch.
>
> 5640 if (ksmbd_stream_fd(fp))
>
> 4. Jumping to label next.
>
> 5641 goto next;
> 5642
> 5643 user_ns = file_mnt_user_ns(fp->filp);
> 5644 parent = dget_parent(dentry);
> 5645 ret = ksmbd_vfs_lock_parent(user_ns, parent, dentry);
> 5646 if (ret) {
> 5647 dput(parent);
> 5648 return ret;
> 5649 }
> 5650
> 5651 parent_fp = ksmbd_lookup_fd_inode(d_inode(parent));
> 5652 inode_unlock(d_inode(parent));
> 5653 dput(parent);
> 5654
> 5655 if (parent_fp) {
> 5656 if (parent_fp->daccess & FILE_DELETE_LE) {
> 5657 pr_err("parent dir is opened with delete
> access\n");
> 5658 return -ESHARE;
> 5659 }
> 5660 }
> 5661 next:
>
> Uninitialized pointer read (UNINIT)
> 5. uninit_use_in_call: Using uninitialized value user_ns when
> calling smb2_rename.
>
> 5662 return smb2_rename(work, fp, user_ns,
> 5663 (struct smb2_file_rename_info *)buf,
> 5664 work->sess->conn->local_nls);
> 5665 }
>
> The pointer user_ns is not initialized and the goto on line 5641 will
> cause this uninitialized pointer value to be passed to smb2_rename.
>
> I suspect user_ns = file_mnt_user_ns(fp->filp) should be moved to line
> 5639 as a fix, but I'm concerned that this is not a suitable fix.
Thanks for the report, Colin.
No, the fix you propose is correct. Ugh, the goto in this function is
rather messy:
if (ksmbd_stream_fd(fp))
goto next;
user_ns = file_mnt_user_ns(fp->filp);
parent = dget_parent(dentry);
ret = ksmbd_vfs_lock_parent(user_ns, parent, dentry);
I'll send a fixed version of this patch later today.
Thanks!
Christian
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-08-19 14:15 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-19 13:11 ksmbd: fix lookup on idmapped mounts Colin Ian King
2021-08-19 14:15 ` Christian Brauner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).