* [bug report] cifsd: add server-side procedures for SMB3
@ 2021-07-08 11:30 Dan Carpenter
0 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2021-07-08 11:30 UTC (permalink / raw)
To: namjae.jeon; +Cc: linux-cifs
Hello Namjae Jeon,
The patch e2f34481b24d: "cifsd: add server-side procedures for SMB3"
from Mar 16, 2021, leads to the following static checker warning:
fs/ksmbd/smb2pdu.c:2329 smb2_create_sd_buffer()
warn: 'context' is an error pointer or valid
fs/ksmbd/smb2pdu.c
2317 static int smb2_create_sd_buffer(struct ksmbd_work *work,
2318 struct smb2_create_req *req,
2319 struct path *path)
2320 {
2321 struct create_context *context;
2322 int rc = -ENOENT;
2323
2324 if (!req->CreateContextsOffset)
2325 return rc;
2326
2327 /* Parse SD BUFFER create contexts */
2328 context = smb2_find_context_vals(req, SMB2_CREATE_SD_BUFFER);
The comments for smb2_find_context_vals() says that it returns NULL on
error but really it never returns NULL.
When a function returns both error pointers and NULL, then NULL means
the feature has been deliberately disabled. Or another use might be
if "p = get_next();" and get_next() will return -ENOMEM if there is an
allocation error but NULL when there aren't any more items. In other
words NULL is a sort of success.
It's better to always write error handling instead of success handling
because normally the error handling is shorter (cleanup and return an
error code) but the success case path is more involved. Also it results
in everything being less indented. Also preserve the error code.
if (IS_ERR(context))
return PTR_ERR(context);
ksmbd_debug(SMB, "Set ACLs using SMB2_CREATE_SD_BUFFER context\n");
2329 if (context && !IS_ERR(context)) {
2330 struct create_sd_buf_req *sd_buf;
2331
2332 ksmbd_debug(SMB,
2333 "Set ACLs using SMB2_CREATE_SD_BUFFER context\n");
2334 sd_buf = (struct create_sd_buf_req *)context;
2335 rc = set_info_sec(work->conn, work->tcon,
2336 path, &sd_buf->ntsd,
2337 le32_to_cpu(sd_buf->ccontext.DataLength), true);
2338 }
2339
2340 return rc;
2341 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* [bug report] cifsd: add server-side procedures for SMB3
@ 2021-11-30 11:54 Dan Carpenter
2021-11-30 23:59 ` Hyunchul Lee
2021-12-01 1:57 ` Namjae Jeon
0 siblings, 2 replies; 7+ messages in thread
From: Dan Carpenter @ 2021-11-30 11:54 UTC (permalink / raw)
To: namjae.jeon; +Cc: linux-cifs
Hello Namjae Jeon,
The patch e2f34481b24d: "cifsd: add server-side procedures for SMB3"
from Mar 16, 2021, leads to the following Smatch static checker
warning:
fs/ksmbd/smb2pdu.c:2970 smb2_open()
error: uninitialized symbol 'pntsd_size'.
fs/ksmbd/smb2pdu.c
2930 if (rc) {
2931 rc = smb2_create_sd_buffer(work, req, &path);
2932 if (rc) {
2933 if (posix_acl_rc)
2934 ksmbd_vfs_set_init_posix_acl(user_ns,
2935 inode);
2936
2937 if (test_share_config_flag(work->tcon->share_conf,
2938 KSMBD_SHARE_FLAG_ACL_XATTR)) {
2939 struct smb_fattr fattr;
2940 struct smb_ntsd *pntsd;
2941 int pntsd_size, ace_num = 0;
2942
2943 ksmbd_acls_fattr(&fattr, user_ns, inode);
2944 if (fattr.cf_acls)
2945 ace_num = fattr.cf_acls->a_count;
2946 if (fattr.cf_dacls)
2947 ace_num += fattr.cf_dacls->a_count;
2948
2949 pntsd = kmalloc(sizeof(struct smb_ntsd) +
2950 sizeof(struct smb_sid) * 3 +
2951 sizeof(struct smb_acl) +
2952 sizeof(struct smb_ace) * ace_num * 2,
2953 GFP_KERNEL);
2954 if (!pntsd)
2955 goto err_out;
2956
2957 rc = build_sec_desc(user_ns,
2958 pntsd, NULL,
2959 OWNER_SECINFO |
2960 GROUP_SECINFO |
2961 DACL_SECINFO,
2962 &pntsd_size, &fattr);
No check for if "rc" is an error code.
2963 posix_acl_release(fattr.cf_acls);
2964 posix_acl_release(fattr.cf_dacls);
2965
2966 rc = ksmbd_vfs_set_sd_xattr(conn,
2967 user_ns,
2968 path.dentry,
2969 pntsd,
--> 2970 pntsd_size);
^^^^^^^^^^
2971 kfree(pntsd);
2972 if (rc)
2973 pr_err("failed to store ntacl in xattr : %d\n",
2974 rc);
2975 }
2976 }
2977 }
2978 rc = 0;
2979 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] cifsd: add server-side procedures for SMB3
2021-11-30 11:54 Dan Carpenter
@ 2021-11-30 23:59 ` Hyunchul Lee
2021-12-01 1:57 ` Namjae Jeon
1 sibling, 0 replies; 7+ messages in thread
From: Hyunchul Lee @ 2021-11-30 23:59 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Namjae Jeon, linux-cifs
Hello Dan,
I will fix it.
Thank you for your report!
2021년 12월 1일 (수) 오전 8:27, Dan Carpenter <dan.carpenter@oracle.com>님이 작성:
>
> Hello Namjae Jeon,
>
> The patch e2f34481b24d: "cifsd: add server-side procedures for SMB3"
> from Mar 16, 2021, leads to the following Smatch static checker
> warning:
>
> fs/ksmbd/smb2pdu.c:2970 smb2_open()
> error: uninitialized symbol 'pntsd_size'.
>
> fs/ksmbd/smb2pdu.c
> 2930 if (rc) {
> 2931 rc = smb2_create_sd_buffer(work, req, &path);
> 2932 if (rc) {
> 2933 if (posix_acl_rc)
> 2934 ksmbd_vfs_set_init_posix_acl(user_ns,
> 2935 inode);
> 2936
> 2937 if (test_share_config_flag(work->tcon->share_conf,
> 2938 KSMBD_SHARE_FLAG_ACL_XATTR)) {
> 2939 struct smb_fattr fattr;
> 2940 struct smb_ntsd *pntsd;
> 2941 int pntsd_size, ace_num = 0;
> 2942
> 2943 ksmbd_acls_fattr(&fattr, user_ns, inode);
> 2944 if (fattr.cf_acls)
> 2945 ace_num = fattr.cf_acls->a_count;
> 2946 if (fattr.cf_dacls)
> 2947 ace_num += fattr.cf_dacls->a_count;
> 2948
> 2949 pntsd = kmalloc(sizeof(struct smb_ntsd) +
> 2950 sizeof(struct smb_sid) * 3 +
> 2951 sizeof(struct smb_acl) +
> 2952 sizeof(struct smb_ace) * ace_num * 2,
> 2953 GFP_KERNEL);
> 2954 if (!pntsd)
> 2955 goto err_out;
> 2956
> 2957 rc = build_sec_desc(user_ns,
> 2958 pntsd, NULL,
> 2959 OWNER_SECINFO |
> 2960 GROUP_SECINFO |
> 2961 DACL_SECINFO,
> 2962 &pntsd_size, &fattr);
>
> No check for if "rc" is an error code.
>
> 2963 posix_acl_release(fattr.cf_acls);
> 2964 posix_acl_release(fattr.cf_dacls);
> 2965
> 2966 rc = ksmbd_vfs_set_sd_xattr(conn,
> 2967 user_ns,
> 2968 path.dentry,
> 2969 pntsd,
> --> 2970 pntsd_size);
> ^^^^^^^^^^
>
> 2971 kfree(pntsd);
> 2972 if (rc)
> 2973 pr_err("failed to store ntacl in xattr : %d\n",
> 2974 rc);
> 2975 }
> 2976 }
> 2977 }
> 2978 rc = 0;
> 2979 }
>
> regards,
> dan carpenter
--
Thanks,
Hyunchul
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] cifsd: add server-side procedures for SMB3
2021-11-30 11:54 Dan Carpenter
2021-11-30 23:59 ` Hyunchul Lee
@ 2021-12-01 1:57 ` Namjae Jeon
1 sibling, 0 replies; 7+ messages in thread
From: Namjae Jeon @ 2021-12-01 1:57 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-cifs
2021-11-30 20:54 GMT+09:00, Dan Carpenter <dan.carpenter@oracle.com>:
> Hello Namjae Jeon,
Hi Dan,
>
> The patch e2f34481b24d: "cifsd: add server-side procedures for SMB3"
> from Mar 16, 2021, leads to the following Smatch static checker
> warning:
>
> fs/ksmbd/smb2pdu.c:2970 smb2_open()
> error: uninitialized symbol 'pntsd_size'.
Thanks for your report! I have sent the patch to the list.
Let me know if the patch doesn't fix this warning:)
>
> fs/ksmbd/smb2pdu.c
> 2930 if (rc) {
> 2931 rc = smb2_create_sd_buffer(work, req,
> &path);
> 2932 if (rc) {
> 2933 if (posix_acl_rc)
> 2934
> ksmbd_vfs_set_init_posix_acl(user_ns,
> 2935
> inode);
> 2936
> 2937 if
> (test_share_config_flag(work->tcon->share_conf,
> 2938
> KSMBD_SHARE_FLAG_ACL_XATTR)) {
> 2939 struct smb_fattr fattr;
> 2940 struct smb_ntsd *pntsd;
> 2941 int pntsd_size, ace_num =
> 0;
> 2942
> 2943 ksmbd_acls_fattr(&fattr,
> user_ns, inode);
> 2944 if (fattr.cf_acls)
> 2945 ace_num =
> fattr.cf_acls->a_count;
> 2946 if (fattr.cf_dacls)
> 2947 ace_num +=
> fattr.cf_dacls->a_count;
> 2948
> 2949 pntsd =
> kmalloc(sizeof(struct smb_ntsd) +
> 2950
> sizeof(struct smb_sid) * 3 +
> 2951
> sizeof(struct smb_acl) +
> 2952
> sizeof(struct smb_ace) * ace_num * 2,
> 2953
> GFP_KERNEL);
> 2954 if (!pntsd)
> 2955 goto err_out;
> 2956
> 2957 rc =
> build_sec_desc(user_ns,
> 2958 pntsd,
> NULL,
> 2959
> OWNER_SECINFO |
> 2960
> GROUP_SECINFO |
> 2961
> DACL_SECINFO,
> 2962
> &pntsd_size, &fattr);
>
> No check for if "rc" is an error code.
>
> 2963
> posix_acl_release(fattr.cf_acls);
> 2964
> posix_acl_release(fattr.cf_dacls);
> 2965
> 2966 rc =
> ksmbd_vfs_set_sd_xattr(conn,
> 2967
> user_ns,
> 2968
> path.dentry,
> 2969
> pntsd,
> --> 2970
> pntsd_size);
>
> ^^^^^^^^^^
>
> 2971 kfree(pntsd);
> 2972 if (rc)
> 2973 pr_err("failed to
> store ntacl in xattr : %d\n",
> 2974 rc);
> 2975 }
> 2976 }
> 2977 }
> 2978 rc = 0;
> 2979 }
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [bug report] cifsd: add server-side procedures for SMB3
@ 2023-05-26 11:56 Dan Carpenter
2023-05-26 14:38 ` Namjae Jeon
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2023-05-26 11:56 UTC (permalink / raw)
To: namjae.jeon; +Cc: linux-cifs
Hello Namjae Jeon,
The patch e2f34481b24d: "cifsd: add server-side procedures for SMB3"
from Mar 16, 2021, leads to the following Smatch static checker
warning:
fs/smb/server/smbacl.c:1296 smb_check_perm_dacl()
error: 'posix_acls' dereferencing possible ERR_PTR()
fs/smb/server/vfs.c:1323 ksmbd_vfs_make_xattr_posix_acl()
error: 'posix_acls' dereferencing possible ERR_PTR()
fs/smb/server/vfs.c:1830 ksmbd_vfs_inherit_posix_acl()
error: 'acls' dereferencing possible ERR_PTR()
fs/smb/server/smbacl.c
1281 if (*pdaccess & FILE_MAXIMAL_ACCESS_LE && found) {
1282 granted = READ_CONTROL | WRITE_DAC | FILE_READ_ATTRIBUTES |
1283 DELETE;
1284
1285 granted |= le32_to_cpu(ace->access_req);
1286
1287 if (!pdacl->num_aces)
1288 granted = GENERIC_ALL_FLAGS;
1289 }
1290
1291 if (IS_ENABLED(CONFIG_FS_POSIX_ACL)) {
1292 posix_acls = get_inode_acl(d_inode(path->dentry), ACL_TYPE_ACCESS);
__get_acl() returns a mix of error pointers and NULL. I don't really
understand the rules here. There are no comments explaining it.
1293 if (posix_acls && !found) {
1294 unsigned int id = -1;
1295
--> 1296 pa_entry = posix_acls->a_entries;
^^^^^^^^^^^^
Potential error pointer dereference.
1297 for (i = 0; i < posix_acls->a_count; i++, pa_entry++) {
1298 if (pa_entry->e_tag == ACL_USER)
1299 id = posix_acl_uid_translate(idmap, pa_entry);
1300 else if (pa_entry->e_tag == ACL_GROUP)
1301 id = posix_acl_gid_translate(idmap, pa_entry);
1302 else
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] cifsd: add server-side procedures for SMB3
2023-05-26 11:56 Dan Carpenter
@ 2023-05-26 14:38 ` Namjae Jeon
0 siblings, 0 replies; 7+ messages in thread
From: Namjae Jeon @ 2023-05-26 14:38 UTC (permalink / raw)
To: Dan Carpenter; +Cc: namjae.jeon, linux-cifs
2023-05-26 20:56 GMT+09:00, Dan Carpenter <dan.carpenter@linaro.org>:
> Hello Namjae Jeon,
>
> The patch e2f34481b24d: "cifsd: add server-side procedures for SMB3"
> from Mar 16, 2021, leads to the following Smatch static checker
> warning:
>
> fs/smb/server/smbacl.c:1296 smb_check_perm_dacl()
> error: 'posix_acls' dereferencing possible ERR_PTR()
> fs/smb/server/vfs.c:1323 ksmbd_vfs_make_xattr_posix_acl()
> error: 'posix_acls' dereferencing possible ERR_PTR()
> fs/smb/server/vfs.c:1830 ksmbd_vfs_inherit_posix_acl()
> error: 'acls' dereferencing possible ERR_PTR()
I will fix it.
Thanks for your report!
>
> fs/smb/server/smbacl.c
> 1281 if (*pdaccess & FILE_MAXIMAL_ACCESS_LE && found) {
> 1282 granted = READ_CONTROL | WRITE_DAC |
> FILE_READ_ATTRIBUTES |
> 1283 DELETE;
> 1284
> 1285 granted |= le32_to_cpu(ace->access_req);
> 1286
> 1287 if (!pdacl->num_aces)
> 1288 granted = GENERIC_ALL_FLAGS;
> 1289 }
> 1290
> 1291 if (IS_ENABLED(CONFIG_FS_POSIX_ACL)) {
> 1292 posix_acls = get_inode_acl(d_inode(path->dentry),
> ACL_TYPE_ACCESS);
>
> __get_acl() returns a mix of error pointers and NULL. I don't really
> understand the rules here. There are no comments explaining it.
>
> 1293 if (posix_acls && !found) {
> 1294 unsigned int id = -1;
> 1295
> --> 1296 pa_entry = posix_acls->a_entries;
> ^^^^^^^^^^^^
> Potential error pointer dereference.
>
> 1297 for (i = 0; i < posix_acls->a_count; i++,
> pa_entry++) {
> 1298 if (pa_entry->e_tag == ACL_USER)
> 1299 id =
> posix_acl_uid_translate(idmap, pa_entry);
> 1300 else if (pa_entry->e_tag ==
> ACL_GROUP)
> 1301 id =
> posix_acl_gid_translate(idmap, pa_entry);
> 1302 else
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [bug report] cifsd: add server-side procedures for SMB3
@ 2024-07-19 23:55 Dan Carpenter
0 siblings, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2024-07-19 23:55 UTC (permalink / raw)
To: Namjae Jeon; +Cc: linux-cifs
Hello Namjae Jeon,
Commit e2f34481b24d ("cifsd: add server-side procedures for SMB3")
from Mar 16, 2021 (linux-next), leads to the following Smatch static
checker warning:
fs/smb/server/smb2pdu.c:8864 smb3_preauth_hash_rsp()
error: we previously assumed 'conn->preauth_info' could be null (see line 8844)
fs/smb/server/smb2pdu.c
8832 void smb3_preauth_hash_rsp(struct ksmbd_work *work)
8833 {
8834 struct ksmbd_conn *conn = work->conn;
8835 struct ksmbd_session *sess = work->sess;
8836 struct smb2_hdr *req, *rsp;
8837
8838 if (conn->dialect != SMB311_PROT_ID)
8839 return;
8840
8841 WORK_BUFFERS(work, req, rsp);
8842
8843 if (le16_to_cpu(req->Command) == SMB2_NEGOTIATE_HE &&
8844 conn->preauth_info)
^^^^^^^^^^^^^^^^^^
This checks for NULL for ksmbd_gen_preauth_integrity_hash().
8845 ksmbd_gen_preauth_integrity_hash(conn, work->response_buf,
8846 conn->preauth_info->Preauth_HashValue);
8847
8848 if (le16_to_cpu(rsp->Command) == SMB2_SESSION_SETUP_HE && sess) {
8849 __u8 *hash_value;
8850
8851 if (conn->binding) {
8852 struct preauth_session *preauth_sess;
8853
8854 preauth_sess = ksmbd_preauth_session_lookup(conn, sess->id);
8855 if (!preauth_sess)
8856 return;
8857 hash_value = preauth_sess->Preauth_HashValue;
8858 } else {
8859 hash_value = sess->Preauth_HashValue;
8860 if (!hash_value)
8861 return;
8862 }
8863 ksmbd_gen_preauth_integrity_hash(conn, work->response_buf,
^^^^
This call doesn't.
--> 8864 hash_value);
8865 }
8866 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-07-19 23:55 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-07-08 11:30 [bug report] cifsd: add server-side procedures for SMB3 Dan Carpenter
-- strict thread matches above, loose matches on Subject: below --
2021-11-30 11:54 Dan Carpenter
2021-11-30 23:59 ` Hyunchul Lee
2021-12-01 1:57 ` Namjae Jeon
2023-05-26 11:56 Dan Carpenter
2023-05-26 14:38 ` Namjae Jeon
2024-07-19 23:55 Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox