* [PATCH] ksmbd: potential uninitialized error code in set_file_basic_info()
@ 2021-09-07 7:33 Dan Carpenter
2021-09-07 8:01 ` Sergey Senozhatsky
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2021-09-07 7:33 UTC (permalink / raw)
To: Namjae Jeon, Christian Brauner
Cc: Sergey Senozhatsky, Steve French, Hyunchul Lee, linux-cifs,
kernel-janitors
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;
--
2.20.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] ksmbd: potential uninitialized error code in set_file_basic_info()
2021-09-07 7:33 [PATCH] ksmbd: potential uninitialized error code in set_file_basic_info() Dan Carpenter
@ 2021-09-07 8:01 ` Sergey Senozhatsky
2021-09-07 8:09 ` Namjae Jeon
2021-09-07 8:48 ` Dan Carpenter
0 siblings, 2 replies; 7+ messages in thread
From: Sergey Senozhatsky @ 2021-09-07 8:01 UTC (permalink / raw)
To: Dan Carpenter
Cc: Namjae Jeon, Christian Brauner, Sergey Senozhatsky, Steve French,
Hyunchul Lee, linux-cifs, kernel-janitors
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ksmbd: potential uninitialized error code in set_file_basic_info()
2021-09-07 8:01 ` Sergey Senozhatsky
@ 2021-09-07 8:09 ` Namjae Jeon
2021-09-07 8:38 ` Sergey Senozhatsky
2021-09-13 9:47 ` Christian Brauner
2021-09-07 8:48 ` Dan Carpenter
1 sibling, 2 replies; 7+ messages in thread
From: Namjae Jeon @ 2021-09-07 8:09 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Dan Carpenter, Christian Brauner, Steve French, Hyunchul Lee,
linux-cifs, kernel-janitors
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!
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ksmbd: potential uninitialized error code in set_file_basic_info()
2021-09-07 8:09 ` Namjae Jeon
@ 2021-09-07 8:38 ` Sergey Senozhatsky
2021-09-13 9:47 ` Christian Brauner
1 sibling, 0 replies; 7+ messages in thread
From: Sergey Senozhatsky @ 2021-09-07 8:38 UTC (permalink / raw)
To: Namjae Jeon
Cc: Sergey Senozhatsky, Dan Carpenter, Christian Brauner,
Steve French, Hyunchul Lee, linux-cifs, kernel-janitors
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;
---
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ksmbd: potential uninitialized error code in set_file_basic_info()
2021-09-07 8:01 ` Sergey Senozhatsky
2021-09-07 8:09 ` Namjae Jeon
@ 2021-09-07 8:48 ` Dan Carpenter
2021-09-07 9:04 ` Sergey Senozhatsky
1 sibling, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2021-09-07 8:48 UTC (permalink / raw)
To: Sergey Senozhatsky
Cc: Namjae Jeon, Christian Brauner, Steve French, Hyunchul Lee,
linux-cifs, kernel-janitors
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ksmbd: potential uninitialized error code in set_file_basic_info()
2021-09-07 8:48 ` Dan Carpenter
@ 2021-09-07 9:04 ` Sergey Senozhatsky
0 siblings, 0 replies; 7+ messages in thread
From: Sergey Senozhatsky @ 2021-09-07 9:04 UTC (permalink / raw)
To: Dan Carpenter
Cc: Sergey Senozhatsky, Namjae Jeon, Christian Brauner, Steve French,
Hyunchul Lee, linux-cifs, kernel-janitors
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ksmbd: potential uninitialized error code in set_file_basic_info()
2021-09-07 8:09 ` Namjae Jeon
2021-09-07 8:38 ` Sergey Senozhatsky
@ 2021-09-13 9:47 ` Christian Brauner
1 sibling, 0 replies; 7+ messages in thread
From: Christian Brauner @ 2021-09-13 9:47 UTC (permalink / raw)
To: Namjae Jeon
Cc: Sergey Senozhatsky, Dan Carpenter, Steve French, Hyunchul Lee,
linux-cifs, kernel-janitors
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-09-13 9:47 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-07 7:33 [PATCH] ksmbd: potential uninitialized error code in set_file_basic_info() Dan Carpenter
2021-09-07 8:01 ` Sergey Senozhatsky
2021-09-07 8:09 ` Namjae Jeon
2021-09-07 8:38 ` Sergey Senozhatsky
2021-09-13 9:47 ` Christian Brauner
2021-09-07 8:48 ` Dan Carpenter
2021-09-07 9:04 ` Sergey Senozhatsky
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).