| Message ID | 20210907073340.GC18254@kili (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ksmbd: potential uninitialized error code in set_file_basic_info() | expand |
On (21/09/07 10:33), Dan Carpenter wrote: > > Smatch complains that there are some paths where "rc" is not set. > > Fixes: eb5784f0c6ef ("ksmbd: ensure error is surfaced in set_file_basic_info()") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > fs/ksmbd/smb2pdu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c > index a350e1cef7f4..c86164dc70bb 100644 > --- a/fs/ksmbd/smb2pdu.c > +++ b/fs/ksmbd/smb2pdu.c > @@ -5444,7 +5444,7 @@ static int set_file_basic_info(struct ksmbd_file *fp, char *buf, > struct file *filp; > struct inode *inode; > struct user_namespace *user_ns; > - int rc; > + int rc = 0; > > if (!(fp->daccess & FILE_WRITE_ATTRIBUTES_LE)) > return -EACCES; It sort of feels like that `rc' is not needed there at all. It's being used in rc = ksmbd_vfs_set_dos_attrib_xattr(user_ns, filp->f_path.dentry, &da); if (rc) ksmbd_debug(SMB, "failed to restore file attribute in EA\n"); and in rc = setattr_prepare(user_ns, dentry, &attrs); if (rc) return -EINVAL; Either it should be used more, and probably be a return value, or we can just remove it.
2021-09-07 17:01 GMT+09:00, Sergey Senozhatsky <senozhatsky@chromium.org>: > On (21/09/07 10:33), Dan Carpenter wrote: >> >> Smatch complains that there are some paths where "rc" is not set. >> >> Fixes: eb5784f0c6ef ("ksmbd: ensure error is surfaced in >> set_file_basic_info()") >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >> --- >> fs/ksmbd/smb2pdu.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c >> index a350e1cef7f4..c86164dc70bb 100644 >> --- a/fs/ksmbd/smb2pdu.c >> +++ b/fs/ksmbd/smb2pdu.c >> @@ -5444,7 +5444,7 @@ static int set_file_basic_info(struct ksmbd_file >> *fp, char *buf, >> struct file *filp; >> struct inode *inode; >> struct user_namespace *user_ns; >> - int rc; >> + int rc = 0; >> >> if (!(fp->daccess & FILE_WRITE_ATTRIBUTES_LE)) >> return -EACCES; > > It sort of feels like that `rc' is not needed there at all. It's being used > in > > rc = ksmbd_vfs_set_dos_attrib_xattr(user_ns, > filp->f_path.dentry, > &da); > if (rc) > ksmbd_debug(SMB, > "failed to restore file attribute in > EA\n"); > > and in > > rc = setattr_prepare(user_ns, dentry, &attrs); > if (rc) > return -EINVAL; > > Either it should be used more, and probably be a return value, or we can > just remove it. This patch is correct. But I have already fixed it. You can understand it if you check #ksmbd-for-next branch, not master. https://git.samba.org/?p=ksmbd.git;a=shortlog;h=refs/heads/ksmbd-for-next Thanks! >
On (21/09/07 17:09), Namjae Jeon wrote: > 2021-09-07 17:01 GMT+09:00, Sergey Senozhatsky <senozhatsky@chromium.org>: > > On (21/09/07 10:33), Dan Carpenter wrote: > >> > >> Smatch complains that there are some paths where "rc" is not set. > >> > >> Fixes: eb5784f0c6ef ("ksmbd: ensure error is surfaced in > >> set_file_basic_info()") > >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > >> --- > >> fs/ksmbd/smb2pdu.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c > >> index a350e1cef7f4..c86164dc70bb 100644 > >> --- a/fs/ksmbd/smb2pdu.c > >> +++ b/fs/ksmbd/smb2pdu.c > >> @@ -5444,7 +5444,7 @@ static int set_file_basic_info(struct ksmbd_file > >> *fp, char *buf, > >> struct file *filp; > >> struct inode *inode; > >> struct user_namespace *user_ns; > >> - int rc; > >> + int rc = 0; > >> > >> if (!(fp->daccess & FILE_WRITE_ATTRIBUTES_LE)) > >> return -EACCES; > > > > It sort of feels like that `rc' is not needed there at all. It's being used > > in > > > > rc = ksmbd_vfs_set_dos_attrib_xattr(user_ns, > > filp->f_path.dentry, > > &da); > > if (rc) > > ksmbd_debug(SMB, > > "failed to restore file attribute in > > EA\n"); > > > > and in > > > > rc = setattr_prepare(user_ns, dentry, &attrs); > > if (rc) > > return -EINVAL; > > > > Either it should be used more, and probably be a return value, or we can > > just remove it. > This patch is correct. But I have already fixed it. > You can understand it if you check #ksmbd-for-next branch, not master. > > https://git.samba.org/?p=ksmbd.git;a=shortlog;h=refs/heads/ksmbd-for-next I assume it's "ksmbd: ensure error is surfaced in set_file_basic_info()" If none of the branches that set `rc' is taken then function returns random stack value: --- int rc; if (test_share_config_flag(share, KSMBD_SHARE_FLAG_STORE_DOS_ATTRS) ... { rc = ... } if (attrs.ia_valid) .... { rc = ... } return rc; ---
On Tue, Sep 07, 2021 at 05:01:11PM +0900, Sergey Senozhatsky wrote: > > rc = setattr_prepare(user_ns, dentry, &attrs); > if (rc) > return -EINVAL; > > Either it should be used more, and probably be a return value, or we can > just remove it. You are looking at old code from before the bug was introduced. regards, dan carpenter
On (21/09/07 11:48), Dan Carpenter wrote: > On Tue, Sep 07, 2021 at 05:01:11PM +0900, Sergey Senozhatsky wrote: > > > > rc = setattr_prepare(user_ns, dentry, &attrs); > > if (rc) > > return -EINVAL; > > > > Either it should be used more, and probably be a return value, or we can > > just remove it. > > You are looking at old code from before the bug was introduced. Right. I fetched today's linux-next and see the point now.
On Tue, Sep 07, 2021 at 05:09:08PM +0900, Namjae Jeon wrote: > 2021-09-07 17:01 GMT+09:00, Sergey Senozhatsky <senozhatsky@chromium.org>: > > On (21/09/07 10:33), Dan Carpenter wrote: > >> > >> Smatch complains that there are some paths where "rc" is not set. > >> > >> Fixes: eb5784f0c6ef ("ksmbd: ensure error is surfaced in > >> set_file_basic_info()") > >> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > >> --- > >> fs/ksmbd/smb2pdu.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c > >> index a350e1cef7f4..c86164dc70bb 100644 > >> --- a/fs/ksmbd/smb2pdu.c > >> +++ b/fs/ksmbd/smb2pdu.c > >> @@ -5444,7 +5444,7 @@ static int set_file_basic_info(struct ksmbd_file > >> *fp, char *buf, > >> struct file *filp; > >> struct inode *inode; > >> struct user_namespace *user_ns; > >> - int rc; > >> + int rc = 0; > >> > >> if (!(fp->daccess & FILE_WRITE_ATTRIBUTES_LE)) > >> return -EACCES; > > > > It sort of feels like that `rc' is not needed there at all. It's being used > > in > > > > rc = ksmbd_vfs_set_dos_attrib_xattr(user_ns, > > filp->f_path.dentry, > > &da); > > if (rc) > > ksmbd_debug(SMB, > > "failed to restore file attribute in > > EA\n"); > > > > and in > > > > rc = setattr_prepare(user_ns, dentry, &attrs); > > if (rc) > > return -EINVAL; > > > > Either it should be used more, and probably be a return value, or we can > > just remove it. > This patch is correct. But I have already fixed it. > You can understand it if you check #ksmbd-for-next branch, not master. > > https://git.samba.org/?p=ksmbd.git;a=shortlog;h=refs/heads/ksmbd-for-next Thanks for fixing it. I was out on vacation last week. Christian
diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c index a350e1cef7f4..c86164dc70bb 100644 --- a/fs/ksmbd/smb2pdu.c +++ b/fs/ksmbd/smb2pdu.c @@ -5444,7 +5444,7 @@ static int set_file_basic_info(struct ksmbd_file *fp, char *buf, struct file *filp; struct inode *inode; struct user_namespace *user_ns; - int rc; + int rc = 0; if (!(fp->daccess & FILE_WRITE_ATTRIBUTES_LE)) return -EACCES;
Smatch complains that there are some paths where "rc" is not set. Fixes: eb5784f0c6ef ("ksmbd: ensure error is surfaced in set_file_basic_info()") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- fs/ksmbd/smb2pdu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)