* [bug report] cifsd: add server-side procedures for SMB3
@ 2021-07-08 11:30 Dan Carpenter
0 siblings, 0 replies; 6+ 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] 6+ 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; 6+ 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] 6+ 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; 6+ 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] 6+ 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; 6+ 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] 6+ 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; 6+ 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] 6+ 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; 6+ 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] 6+ messages in thread
end of thread, other threads:[~2023-05-26 14:38 UTC | newest]
Thread overview: 6+ 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
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
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).