* [PATCH][CIFS] Fix uninitialized pointer access to dacl_ptr in build_sec_desc @ 2021-06-22 23:02 Steve French 2021-06-23 11:41 ` Aurélien Aptel 0 siblings, 1 reply; 4+ messages in thread From: Steve French @ 2021-06-22 23:02 UTC (permalink / raw) To: CIFS; +Cc: Shyam Prasad N [-- Attachment #1: Type: text/plain, Size: 305 bytes --] smb3: fix possible access to uninitialized pointer to DACL dacl_ptr can be null so we must check for it (ie if dacloffset is set) everywhere dacl_ptr is used in build_sec_desc - and we were missing one check Addresses-Coverity: 1475598 ("Explicit null dereference") -- Thanks, Steve [-- Attachment #2: 0001-smb3-fix-possible-access-to-unitialized-pointer-to-D.patch --] [-- Type: text/x-patch, Size: 1072 bytes --] From ec06cb04376e5abc927a9b85dd768ce8728965bb Mon Sep 17 00:00:00 2001 From: Steve French <stfrench@microsoft.com> Date: Tue, 22 Jun 2021 17:54:50 -0500 Subject: [PATCH] smb3: fix possible access to uninitialized pointer to DACL dacl_ptr can be null so we must check for it everywhere it is used in build_sec_desc. Addresses-Coverity: 1475598 ("Explicit null dereference") Signed-off-by: Steve French <stfrench@microsoft.com> --- fs/cifs/cifsacl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c index 784407f9280f..25a8139336fa 100644 --- a/fs/cifs/cifsacl.c +++ b/fs/cifs/cifsacl.c @@ -1308,7 +1308,7 @@ static int build_sec_desc(struct cifs_ntsd *pntsd, struct cifs_ntsd *pnntsd, ndacl_ptr = (struct cifs_acl *)((char *)pnntsd + ndacloffset); ndacl_ptr->revision = dacloffset ? dacl_ptr->revision : cpu_to_le16(ACL_REVISION); - ndacl_ptr->num_aces = dacl_ptr->num_aces; + ndacl_ptr->num_aces = dacloffset ? dacl_ptr->num_aces : 0; if (uid_valid(uid)) { /* chown */ uid_t id; -- 2.30.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH][CIFS] Fix uninitialized pointer access to dacl_ptr in build_sec_desc 2021-06-22 23:02 [PATCH][CIFS] Fix uninitialized pointer access to dacl_ptr in build_sec_desc Steve French @ 2021-06-23 11:41 ` Aurélien Aptel 2021-06-23 14:17 ` Steve French 0 siblings, 1 reply; 4+ messages in thread From: Aurélien Aptel @ 2021-06-23 11:41 UTC (permalink / raw) To: Steve French, CIFS; +Cc: Shyam Prasad N Steve French <smfrench@gmail.com> writes: > smb3: fix possible access to uninitialized pointer to DACL > > dacl_ptr can be null so we must check for it (ie if dacloffset is > set) everywhere dacl_ptr is > used in build_sec_desc - and we were missing one check > > Addresses-Coverity: 1475598 ("Explicit null dereference") Looks OK since dacl_ptr is only set if dacloffset is set but it would be clearer if you check for dacl_ptr directly no? Any reason you are checking this way? I think this is clearer, unless I'm missing something: ndacl_ptr->num_aces = dacl_ptr ? dacl_ptr->num_aces : 0; > > > -- > Thanks, > > Steve > From ec06cb04376e5abc927a9b85dd768ce8728965bb Mon Sep 17 00:00:00 2001 > From: Steve French <stfrench@microsoft.com> > Date: Tue, 22 Jun 2021 17:54:50 -0500 > Subject: [PATCH] smb3: fix possible access to uninitialized pointer to DACL > > dacl_ptr can be null so we must check for it everywhere it is > used in build_sec_desc. > > Addresses-Coverity: 1475598 ("Explicit null dereference") > Signed-off-by: Steve French <stfrench@microsoft.com> > --- > fs/cifs/cifsacl.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c > index 784407f9280f..25a8139336fa 100644 > --- a/fs/cifs/cifsacl.c > +++ b/fs/cifs/cifsacl.c > @@ -1308,7 +1308,7 @@ static int build_sec_desc(struct cifs_ntsd *pntsd, struct cifs_ntsd *pnntsd, > ndacl_ptr = (struct cifs_acl *)((char *)pnntsd + ndacloffset); > ndacl_ptr->revision = > dacloffset ? dacl_ptr->revision : cpu_to_le16(ACL_REVISION); > - ndacl_ptr->num_aces = dacl_ptr->num_aces; > + ndacl_ptr->num_aces = dacloffset ? dacl_ptr->num_aces : 0; > > if (uid_valid(uid)) { /* chown */ > uid_t id; > -- > 2.30.2 > -- 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: [PATCH][CIFS] Fix uninitialized pointer access to dacl_ptr in build_sec_desc 2021-06-23 11:41 ` Aurélien Aptel @ 2021-06-23 14:17 ` Steve French 2021-06-24 0:27 ` Steve French 0 siblings, 1 reply; 4+ messages in thread From: Steve French @ 2021-06-23 14:17 UTC (permalink / raw) To: Aurélien Aptel; +Cc: CIFS, Shyam Prasad N On Wed, Jun 23, 2021 at 6:41 AM Aurélien Aptel <aaptel@suse.com> wrote: > > Steve French <smfrench@gmail.com> writes: > > > smb3: fix possible access to uninitialized pointer to DACL > > > > dacl_ptr can be null so we must check for it (ie if dacloffset is > > set) everywhere dacl_ptr is > > used in build_sec_desc - and we were missing one check > > > > Addresses-Coverity: 1475598 ("Explicit null dereference") > > Looks OK since dacl_ptr is only set if dacloffset is set but it would > be clearer if you check for dacl_ptr directly no? Any reason you are > checking this way? > > I think this is clearer, unless I'm missing something: > > ndacl_ptr->num_aces = dacl_ptr ? dacl_ptr->num_aces : 0; I agree that your suggestion is clearer but I was trying to match the existing checks in the same code. Will change both to your suggestion which is clearer. > > > > > > -- > > Thanks, > > > > Steve > > From ec06cb04376e5abc927a9b85dd768ce8728965bb Mon Sep 17 00:00:00 2001 > > From: Steve French <stfrench@microsoft.com> > > Date: Tue, 22 Jun 2021 17:54:50 -0500 > > Subject: [PATCH] smb3: fix possible access to uninitialized pointer to DACL > > > > dacl_ptr can be null so we must check for it everywhere it is > > used in build_sec_desc. > > > > Addresses-Coverity: 1475598 ("Explicit null dereference") > > Signed-off-by: Steve French <stfrench@microsoft.com> > > --- > > fs/cifs/cifsacl.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c > > index 784407f9280f..25a8139336fa 100644 > > --- a/fs/cifs/cifsacl.c > > +++ b/fs/cifs/cifsacl.c > > @@ -1308,7 +1308,7 @@ static int build_sec_desc(struct cifs_ntsd *pntsd, struct cifs_ntsd *pnntsd, > > ndacl_ptr = (struct cifs_acl *)((char *)pnntsd + ndacloffset); > > ndacl_ptr->revision = > > dacloffset ? dacl_ptr->revision : cpu_to_le16(ACL_REVISION); > > - ndacl_ptr->num_aces = dacl_ptr->num_aces; > > + ndacl_ptr->num_aces = dacloffset ? dacl_ptr->num_aces : 0; > > > > if (uid_valid(uid)) { /* chown */ > > uid_t id; > > -- > > 2.30.2 > > > > -- > 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: [PATCH][CIFS] Fix uninitialized pointer access to dacl_ptr in build_sec_desc 2021-06-23 14:17 ` Steve French @ 2021-06-24 0:27 ` Steve French 0 siblings, 0 replies; 4+ messages in thread From: Steve French @ 2021-06-24 0:27 UTC (permalink / raw) To: Aurélien Aptel; +Cc: CIFS, Shyam Prasad N [-- Attachment #1: Type: text/plain, Size: 2882 bytes --] just changed the one place you noted (to keep the patch smaller) - see attached. On Wed, Jun 23, 2021 at 9:17 AM Steve French <smfrench@gmail.com> wrote: > > On Wed, Jun 23, 2021 at 6:41 AM Aurélien Aptel <aaptel@suse.com> wrote: > > > > Steve French <smfrench@gmail.com> writes: > > > > > smb3: fix possible access to uninitialized pointer to DACL > > > > > > dacl_ptr can be null so we must check for it (ie if dacloffset is > > > set) everywhere dacl_ptr is > > > used in build_sec_desc - and we were missing one check > > > > > > Addresses-Coverity: 1475598 ("Explicit null dereference") > > > > Looks OK since dacl_ptr is only set if dacloffset is set but it would > > be clearer if you check for dacl_ptr directly no? Any reason you are > > checking this way? > > > > I think this is clearer, unless I'm missing something: > > > > ndacl_ptr->num_aces = dacl_ptr ? dacl_ptr->num_aces : 0; > > I agree that your suggestion is clearer but I was trying to match the > existing checks in the same code. > Will change both to your suggestion which is clearer. > > > > > > > > > > > -- > > > Thanks, > > > > > > Steve > > > From ec06cb04376e5abc927a9b85dd768ce8728965bb Mon Sep 17 00:00:00 2001 > > > From: Steve French <stfrench@microsoft.com> > > > Date: Tue, 22 Jun 2021 17:54:50 -0500 > > > Subject: [PATCH] smb3: fix possible access to uninitialized pointer to DACL > > > > > > dacl_ptr can be null so we must check for it everywhere it is > > > used in build_sec_desc. > > > > > > Addresses-Coverity: 1475598 ("Explicit null dereference") > > > Signed-off-by: Steve French <stfrench@microsoft.com> > > > --- > > > fs/cifs/cifsacl.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c > > > index 784407f9280f..25a8139336fa 100644 > > > --- a/fs/cifs/cifsacl.c > > > +++ b/fs/cifs/cifsacl.c > > > @@ -1308,7 +1308,7 @@ static int build_sec_desc(struct cifs_ntsd *pntsd, struct cifs_ntsd *pnntsd, > > > ndacl_ptr = (struct cifs_acl *)((char *)pnntsd + ndacloffset); > > > ndacl_ptr->revision = > > > dacloffset ? dacl_ptr->revision : cpu_to_le16(ACL_REVISION); > > > - ndacl_ptr->num_aces = dacl_ptr->num_aces; > > > + ndacl_ptr->num_aces = dacloffset ? dacl_ptr->num_aces : 0; > > > > > > if (uid_valid(uid)) { /* chown */ > > > uid_t id; > > > -- > > > 2.30.2 > > > > > > > -- > > 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: 0003-smb3-fix-possible-access-to-uninitialized-pointer-to.patch --] [-- Type: text/x-patch, Size: 1075 bytes --] From db310c562e2ff3275a4ef13d648075a290648159 Mon Sep 17 00:00:00 2001 From: Steve French <stfrench@microsoft.com> Date: Tue, 22 Jun 2021 17:54:50 -0500 Subject: [PATCH 3/5] smb3: fix possible access to uninitialized pointer to DACL dacl_ptr can be null so we must check for it everywhere it is used in build_sec_desc. Addresses-Coverity: 1475598 ("Explicit null dereference") Signed-off-by: Steve French <stfrench@microsoft.com> --- fs/cifs/cifsacl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c index 5ec5d9d24032..3356d762cdf5 100644 --- a/fs/cifs/cifsacl.c +++ b/fs/cifs/cifsacl.c @@ -1294,7 +1294,7 @@ static int build_sec_desc(struct cifs_ntsd *pntsd, struct cifs_ntsd *pnntsd, ndacl_ptr = (struct cifs_acl *)((char *)pnntsd + ndacloffset); ndacl_ptr->revision = dacloffset ? dacl_ptr->revision : cpu_to_le16(ACL_REVISION); - ndacl_ptr->num_aces = dacl_ptr->num_aces; + ndacl_ptr->num_aces = dacl_ptr ? dacl_ptr->num_aces : 0; if (uid_valid(uid)) { /* chown */ uid_t id; -- 2.30.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-06-24 0:28 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-06-22 23:02 [PATCH][CIFS] Fix uninitialized pointer access to dacl_ptr in build_sec_desc Steve French 2021-06-23 11:41 ` Aurélien Aptel 2021-06-23 14:17 ` Steve French 2021-06-24 0:27 ` Steve French
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).