* 2 error cases in sid_to_id are ignored @ 2021-06-21 20:57 Steve French 2021-06-22 10:18 ` Aurélien Aptel 0 siblings, 1 reply; 4+ messages in thread From: Steve French @ 2021-06-21 20:57 UTC (permalink / raw) To: Shyam Prasad N; +Cc: CIFS There are two cases (see below) in sid_to_id where errors occur mapping the uid but the rc which is set is overwritten (reset to 0 before return). saved_cred = override_creds(root_cred); sidkey = request_key(&cifs_idmap_key_type, sidstr, ""); if (IS_ERR(sidkey)) { rc = -EINVAL; cifs_dbg(FYI, "%s: Can't map SID %s to a %cid\n", __func__, sidstr, sidtype == SIDOWNER ? 'u' : 'g'); goto out_revert_creds; } /* * FIXME: Here we assume that uid_t and gid_t are same size. It's * probably a safe assumption but might be better to check based on * sidtype. */ BUILD_BUG_ON(sizeof(uid_t) != sizeof(gid_t)); if (sidkey->datalen != sizeof(uid_t)) { rc = -EIO; cifs_dbg(FYI, "%s: Downcall contained malformed key (datalen=%hu)\n", __func__, sidkey->datalen); key_invalidate(sidkey); goto out_key_put; } since later in the function we do: out_key_put: key_put(sidkey); out_revert_creds: revert_creds(saved_cred); kfree(sidstr); /* * Note that we return 0 here unconditionally. If the mapping * fails then we just fall back to using the ctx->linux_uid/linux_gid. */ got_valid_id: rc = 0; if (sidtype == SIDOWNER) fattr->cf_uid = fuid; else fattr->cf_gid = fgid; return rc; } Any thoughts on whether it would be better to return the errors, or continue the current strategy of simply using the default uid/gid for the mount and returning 0 (and removing the two places above where we set rc to non zero values, since rc will be overwritten with 0)? -- Thanks, Steve ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: 2 error cases in sid_to_id are ignored 2021-06-21 20:57 2 error cases in sid_to_id are ignored Steve French @ 2021-06-22 10:18 ` Aurélien Aptel 2021-06-22 19:05 ` Steve French 0 siblings, 1 reply; 4+ messages in thread From: Aurélien Aptel @ 2021-06-22 10:18 UTC (permalink / raw) To: Steve French, Shyam Prasad N; +Cc: CIFS Steve French <smfrench@gmail.com> writes: > Any thoughts on whether it would be better to return the errors, or > continue the current strategy of simply using the default uid/gid for > the mount and returning 0 (and removing the two places above where we > set rc to non zero values, since rc will be overwritten with 0)? My opinion: I think best-effort would be better, so returning things as default uid/gid (or possibly root). Cheers, -- Aurélien Aptel / SUSE Labs Samba Team GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München) ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: 2 error cases in sid_to_id are ignored 2021-06-22 10:18 ` Aurélien Aptel @ 2021-06-22 19:05 ` Steve French 2021-06-22 19:16 ` Steve French 0 siblings, 1 reply; 4+ messages in thread From: Steve French @ 2021-06-22 19:05 UTC (permalink / raw) To: Aurélien Aptel; +Cc: Shyam Prasad N, CIFS Should we make the error noisier? Perhaps we could add a dynamic trace point for failed id mappings so we could debug these if problems with the upcall, malformed ids/SIDs etc? On Tue, Jun 22, 2021 at 5:18 AM Aurélien Aptel <aaptel@suse.com> wrote: > > Steve French <smfrench@gmail.com> writes: > > Any thoughts on whether it would be better to return the errors, or > > continue the current strategy of simply using the default uid/gid for > > the mount and returning 0 (and removing the two places above where we > > set rc to non zero values, since rc will be overwritten with 0)? > > My opinion: I think best-effort would be better, so returning things as > default uid/gid (or possibly root). > > Cheers, > -- > Aurélien Aptel / SUSE Labs Samba Team > GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3 > SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München) > -- Thanks, Steve ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: 2 error cases in sid_to_id are ignored 2021-06-22 19:05 ` Steve French @ 2021-06-22 19:16 ` Steve French 0 siblings, 0 replies; 4+ messages in thread From: Steve French @ 2021-06-22 19:16 UTC (permalink / raw) To: Aurélien Aptel; +Cc: Shyam Prasad N, CIFS [-- Attachment #1: Type: text/plain, Size: 1257 bytes --] OK - removed the two places where rc is set uselessly. Patch attached On Tue, Jun 22, 2021 at 2:05 PM Steve French <smfrench@gmail.com> wrote: > > Should we make the error noisier? > > Perhaps we could add a dynamic trace point for failed id mappings so > we could debug these if problems with the upcall, malformed ids/SIDs > etc? > > On Tue, Jun 22, 2021 at 5:18 AM Aurélien Aptel <aaptel@suse.com> wrote: > > > > Steve French <smfrench@gmail.com> writes: > > > Any thoughts on whether it would be better to return the errors, or > > > continue the current strategy of simply using the default uid/gid for > > > the mount and returning 0 (and removing the two places above where we > > > set rc to non zero values, since rc will be overwritten with 0)? > > > > My opinion: I think best-effort would be better, so returning things as > > default uid/gid (or possibly root). > > > > Cheers, > > -- > > Aurélien Aptel / SUSE Labs Samba Team > > GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3 > > SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE > > GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München) > > > > > -- > Thanks, > > Steve -- Thanks, Steve [-- Attachment #2: 0001-cifs-remove-two-cases-where-rc-is-set-unnecessarily-.patch --] [-- Type: text/x-patch, Size: 1434 bytes --] From 7c3cbf7dbc5d67ba4b3df4a502fbc2441aedd36a Mon Sep 17 00:00:00 2001 From: Steve French <stfrench@microsoft.com> Date: Tue, 22 Jun 2021 14:07:36 -0500 Subject: [PATCH] cifs: remove two cases where rc is set unnecessarily in sid_to_id In both these cases sid_to_id unconditionally returned success, and used the default uid/gid for the mount, so setting rc is confusing and simply gets overwritten (set to 0) later in the function. Addresses-Coverity: 1491672 ("Unused value") Signed-off-by: Steve French <stfrench@microsoft.com> --- fs/cifs/cifsacl.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c index 3898a9e6d3c6..5ec5d9d24032 100644 --- a/fs/cifs/cifsacl.c +++ b/fs/cifs/cifsacl.c @@ -397,7 +397,6 @@ sid_to_id(struct cifs_sb_info *cifs_sb, struct cifs_sid *psid, saved_cred = override_creds(root_cred); sidkey = request_key(&cifs_idmap_key_type, sidstr, ""); if (IS_ERR(sidkey)) { - rc = -EINVAL; cifs_dbg(FYI, "%s: Can't map SID %s to a %cid\n", __func__, sidstr, sidtype == SIDOWNER ? 'u' : 'g'); goto out_revert_creds; @@ -410,7 +409,6 @@ sid_to_id(struct cifs_sb_info *cifs_sb, struct cifs_sid *psid, */ BUILD_BUG_ON(sizeof(uid_t) != sizeof(gid_t)); if (sidkey->datalen != sizeof(uid_t)) { - rc = -EIO; cifs_dbg(FYI, "%s: Downcall contained malformed key (datalen=%hu)\n", __func__, sidkey->datalen); key_invalidate(sidkey); -- 2.30.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-06-22 19:17 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-21 20:57 2 error cases in sid_to_id are ignored Steve French 2021-06-22 10:18 ` Aurélien Aptel 2021-06-22 19:05 ` Steve French 2021-06-22 19:16 ` Steve French
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.