linux-cifs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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).