[PATCH V2] catch acl==NULL in __jfs_set_acl (fixed null pointer dereference)

[PATCH V2] catch acl==NULL in __jfs_set_acl (fixed null pointer dereference)

[Marco Munderloh]

[-- Attachment #1: Type: text/plain, Size: 809 bytes --]

changes V2: I forgot to set rc = 0, leaving it uninitialized if acl was NULL.

--- linux-3.14.2.vanilla/fs/jfs/acl.c	2014-04-28 17:24:55.544597204 +0200
+++ linux-3.14.2/fs/jfs/acl.c	2014-04-29 19:57:27.028465728 +0200
@@ -83,13 +83,16 @@
 	switch (type) {
 	case ACL_TYPE_ACCESS:
 		ea_name = POSIX_ACL_XATTR_ACCESS;
-		rc = posix_acl_equiv_mode(acl, &inode->i_mode);
-		if (rc < 0)
-			return rc;
-		inode->i_ctime = CURRENT_TIME;
-		mark_inode_dirty(inode);
-		if (rc == 0)
-			acl = NULL;
+		if( acl ) {
+			rc = posix_acl_equiv_mode(acl, &inode->i_mode);
+			if (rc < 0)
+				return rc;
+			inode->i_ctime = CURRENT_TIME;
+			mark_inode_dirty(inode);
+			if (rc == 0)
+				acl = NULL;
+		}
+		rc = 0;
 		break;
 	case ACL_TYPE_DEFAULT:
 		ea_name = POSIX_ACL_XATTR_DEFAULT;


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4700 bytes --]

Re: [PATCH V2] catch acl==NULL in __jfs_set_acl (fixed null pointer dereference)

[Matthew Wilcox]

On Tue, Apr 29, 2014 at 07:59:51PM +0200, Marco Munderloh wrote:
> changes V2: I forgot to set rc = 0, leaving it uninitialized if acl was NULL.

You don't need to initialise rc here.  But why not, more simply:

+++ b/fs/jfs/acl.c
@@ -83,6 +83,8 @@ static int __jfs_set_acl(tid_t tid, struct inode *inode, int t
        switch (type) {
        case ACL_TYPE_ACCESS:
                ea_name = POSIX_ACL_XATTR_ACCESS;
+               if (!acl)
+                       break;
                rc = posix_acl_equiv_mode(acl, &inode->i_mode);
                if (rc < 0)
                        return rc;

Re: [PATCH V2] catch acl==NULL in __jfs_set_acl (fixed null pointer dereference)

[Christoph Hellwig]

On Tue, Apr 29, 2014 at 02:26:02PM -0400, Matthew Wilcox wrote:
> On Tue, Apr 29, 2014 at 07:59:51PM +0200, Marco Munderloh wrote:
> > changes V2: I forgot to set rc = 0, leaving it uninitialized if acl was NULL.
> 
> You don't need to initialise rc here.  But why not, more simply:

I'm pretty sure there's a patch queued up somewhere to make
posix_acl_equiv_mode handle the NULL ACL.  I think this was first
reportd on gfs2.

Re: [PATCH V2] catch acl==NULL in __jfs_set_acl (fixed null pointer dereference)

[Marco Munderloh]

[-- Attachment #1: Type: text/plain, Size: 704 bytes --]

> You don't need to initialise rc here.

Yes, you are true. I missed the __jfs_setxattr before the set_cached_acl.

But why not, more simply:
> 
> +++ b/fs/jfs/acl.c
> @@ -83,6 +83,8 @@ static int __jfs_set_acl(tid_t tid, struct inode *inode, int t
>         switch (type) {
>         case ACL_TYPE_ACCESS:
>                 ea_name = POSIX_ACL_XATTR_ACCESS;
> +               if (!acl)
> +                       break;
>                 rc = posix_acl_equiv_mode(acl, &inode->i_mode);
>                 if (rc < 0)
>                         return rc;

I could have done it like this, but I copied the way it was done in btrfs/acl.c, where the jfs implementation seems to come from.


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4700 bytes --]

Re: [PATCH V2] catch acl==NULL in __jfs_set_acl (fixed null pointer dereference)

[Marco Munderloh]

[-- Attachment #1: Type: text/plain, Size: 363 bytes --]

> I'm pretty sure there's a patch queued up somewhere to make
> posix_acl_equiv_mode handle the NULL ACL.  I think this was first
> reportd on gfs2.

That might be a better solution, posix_acl_equiv is used at several places (e.g. tmpfs, where the bug is also triggered).
However, in e.g. the ext4 or the btrfs acl implementation, acl == NULL is catched.


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4700 bytes --]