[PATCH] audit: vfs: fix audit records error when write to a file

[PATCH] audit: vfs: fix audit records error when write to a file

[hujianyang]

commit 33e2208acfc1

audit: vfs: fix audit_inode call in O_CREAT case of do_last

fix a regression in auditing of open(..., O_CREAT) syscalls but
import a new problem which lead the records of write operation
confusion.

This error can be reproduced by these steps:

	touch /etc/test
	echo "-w /etc/test" >>/etc/audit/audit.rules
	/etc/init.d/auditd restart

	echo "abc" >> /etc/test

audit_name records are:

type=PATH msg=audit(1409764556.196:67): item=0 name="/etc/" inode=5097 dev=00:01 mode=040755 ouid=0 ogid=0 rdev=00:00 nametype=PARENT
type=PATH msg=audit(1409764556.196:67): item=1 name=(null) inode=23161 dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
type=PATH msg=audit(1409764556.196:67): item=2 name=(null) inode=23161 dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL

but if we revert commit 33e2208acfc1, records are correct:

type=PATH msg=audit(1409763058.192:219): item=0 name="/etc/test" inode=1275 dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL

We shouldn't leave audit_inode(.., LOOKUP_PARENT) in O_CREAT case
of do_last() because this branch don't really know if vfs need to
create a new file. There is no need to do vfs_create() if we open
an existing file with O_CREAT flag and write to it. But this
audit_inode() in O_CREAT case will record a msg as we create a new
file and confuse the records of write.

This patch moves the audit for create operation to where a file
really need to be created, the O_CREAT case in lookup_open().
We have to add the pointer of struct filename as a parameter of
lookup_open(). By doing this, the records of both create and write
are correct.

Signed-off-by: hujianyang <hujianyang@huawei.com>
---
 fs/namei.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index a996bb4..0bc7734 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2808,7 +2808,8 @@ looked_up:
 static int lookup_open(struct nameidata *nd, struct path *path,
 			struct file *file,
 			const struct open_flags *op,
-			bool got_write, int *opened)
+			bool got_write, int *opened,
+			struct filename *name)
 {
 	struct dentry *dir = nd->path.dentry;
 	struct inode *dir_inode = dir->d_inode;
@@ -2854,6 +2855,9 @@ static int lookup_open(struct nameidata *nd, struct path *path,
 			error = -EROFS;
 			goto out_dput;
 		}
+
+		audit_inode(name, dir, LOOKUP_PARENT);
+
 		*opened |= FILE_CREATED;
 		error = security_path_mknod(&nd->path, dentry, mode, 0);
 		if (error)
@@ -2926,7 +2930,6 @@ static int do_last(struct nameidata *nd, struct path *path,
 		if (error)
 			return error;

-		audit_inode(name, dir, LOOKUP_PARENT);
 		error = -EISDIR;
 		/* trailing slashes? */
 		if (nd->last.name[nd->last.len])
@@ -2945,7 +2948,7 @@ retry_lookup:
 		 */
 	}
 	mutex_lock(&dir->d_inode->i_mutex);
-	error = lookup_open(nd, path, file, op, got_write, opened);
+	error = lookup_open(nd, path, file, op, got_write, opened, name);
 	mutex_unlock(&dir->d_inode->i_mutex);

 	if (error <= 0) {
-- 
1.8.5.5

Re: [PATCH] audit: vfs: fix audit records error when write to a file

[Jeff Layton]

On Fri, 5 Sep 2014 14:55:53 +0800
hujianyang <hujianyang@huawei.com> wrote:

> commit 33e2208acfc1
> 
> audit: vfs: fix audit_inode call in O_CREAT case of do_last
> 
> fix a regression in auditing of open(..., O_CREAT) syscalls but
> import a new problem which lead the records of write operation
> confusion.
> 
> This error can be reproduced by these steps:
> 
> 	touch /etc/test
> 	echo "-w /etc/test" >>/etc/audit/audit.rules
> 	/etc/init.d/auditd restart
> 
> 	echo "abc" >> /etc/test
> 
> audit_name records are:
> 
> type=PATH msg=audit(1409764556.196:67): item=0 name="/etc/" inode=5097 dev=00:01 mode=040755 ouid=0 ogid=0 rdev=00:00 nametype=PARENT
> type=PATH msg=audit(1409764556.196:67): item=1 name=(null) inode=23161 dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
> type=PATH msg=audit(1409764556.196:67): item=2 name=(null) inode=23161 dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
> 
> but if we revert commit 33e2208acfc1, records are correct:
> 
> type=PATH msg=audit(1409763058.192:219): item=0 name="/etc/test" inode=1275 dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
> 
> We shouldn't leave audit_inode(.., LOOKUP_PARENT) in O_CREAT case
> of do_last() because this branch don't really know if vfs need to
> create a new file. There is no need to do vfs_create() if we open
> an existing file with O_CREAT flag and write to it. But this
> audit_inode() in O_CREAT case will record a msg as we create a new
> file and confuse the records of write.
> 
> This patch moves the audit for create operation to where a file
> really need to be created, the O_CREAT case in lookup_open().
> We have to add the pointer of struct filename as a parameter of
> lookup_open(). By doing this, the records of both create and write
> are correct.
> 
> Signed-off-by: hujianyang <hujianyang@huawei.com>
> ---
>  fs/namei.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index a996bb4..0bc7734 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2808,7 +2808,8 @@ looked_up:
>  static int lookup_open(struct nameidata *nd, struct path *path,
>  			struct file *file,
>  			const struct open_flags *op,
> -			bool got_write, int *opened)
> +			bool got_write, int *opened,
> +			struct filename *name)
>  {
>  	struct dentry *dir = nd->path.dentry;
>  	struct inode *dir_inode = dir->d_inode;
> @@ -2854,6 +2855,9 @@ static int lookup_open(struct nameidata *nd, struct path *path,
>  			error = -EROFS;
>  			goto out_dput;
>  		}
> +
> +		audit_inode(name, dir, LOOKUP_PARENT);
> +
>  		*opened |= FILE_CREATED;
>  		error = security_path_mknod(&nd->path, dentry, mode, 0);
>  		if (error)
> @@ -2926,7 +2930,6 @@ static int do_last(struct nameidata *nd, struct path *path,
>  		if (error)
>  			return error;
> 
> -		audit_inode(name, dir, LOOKUP_PARENT);
>  		error = -EISDIR;
>  		/* trailing slashes? */
>  		if (nd->last.name[nd->last.len])
> @@ -2945,7 +2948,7 @@ retry_lookup:
>  		 */
>  	}
>  	mutex_lock(&dir->d_inode->i_mutex);
> -	error = lookup_open(nd, path, file, op, got_write, opened);
> +	error = lookup_open(nd, path, file, op, got_write, opened, name);
>  	mutex_unlock(&dir->d_inode->i_mutex);
> 
>  	if (error <= 0) {

I'm not sure about this. Won't this cause us to miss creating audit
records in some error conditions?

For instance, if you end up not being able to create the file due to
the fs being read-only, then I think this patch would make you miss the
audit record for the parent.

Might it be better to not plumb an extra pointer into lookup_open and
just move the audit_inode calls around do_last in the appropriate
places instead?

-- 
Jeff Layton <jlayton@primarydata.com>

Re: [PATCH] audit: vfs: fix audit records error when write to a file

[胡剑阳]

Hi Jeff,

I don't know if we can keep audit_inode() in do_last(). I've tried
this but found
the only way to enable both write() and create() is moving it to lookup_open().
I know this is not good, but I think we have no choice.

The current patch really miss the ROFS condition as you said. How about
moving audit_inode() to the beginning of the O_CREAT case in lookup_open()?
That's my mistake. After this, it seems no creating records will be missing.

I would like to change this and resend a new patch. I'm worry about if we miss
something. Before I do this, do you have any suggestions? Or do you have a
better way to keep audit_inode() in do_last()?

Thank you~!

Hu

2014-09-05 18:50 GMT+08:00 Jeff Layton <jeff.layton@primarydata.com>:
> On Fri, 5 Sep 2014 14:55:53 +0800
> hujianyang <hujianyang@huawei.com> wrote:
>
>> commit 33e2208acfc1
>>
>> audit: vfs: fix audit_inode call in O_CREAT case of do_last
>>
>> fix a regression in auditing of open(..., O_CREAT) syscalls but
>> import a new problem which lead the records of write operation
>> confusion.
>>
>> This error can be reproduced by these steps:
>>
>>       touch /etc/test
>>       echo "-w /etc/test" >>/etc/audit/audit.rules
>>       /etc/init.d/auditd restart
>>
>>       echo "abc" >> /etc/test
>>
>> audit_name records are:
>>
>> type=PATH msg=audit(1409764556.196:67): item=0 name="/etc/" inode=5097 dev=00:01 mode=040755 ouid=0 ogid=0 rdev=00:00 nametype=PARENT
>> type=PATH msg=audit(1409764556.196:67): item=1 name=(null) inode=23161 dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
>> type=PATH msg=audit(1409764556.196:67): item=2 name=(null) inode=23161 dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
>>
>> but if we revert commit 33e2208acfc1, records are correct:
>>
>> type=PATH msg=audit(1409763058.192:219): item=0 name="/etc/test" inode=1275 dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
>>
>> We shouldn't leave audit_inode(.., LOOKUP_PARENT) in O_CREAT case
>> of do_last() because this branch don't really know if vfs need to
>> create a new file. There is no need to do vfs_create() if we open
>> an existing file with O_CREAT flag and write to it. But this
>> audit_inode() in O_CREAT case will record a msg as we create a new
>> file and confuse the records of write.
>>
>> This patch moves the audit for create operation to where a file
>> really need to be created, the O_CREAT case in lookup_open().
>> We have to add the pointer of struct filename as a parameter of
>> lookup_open(). By doing this, the records of both create and write
>> are correct.
>>
>> Signed-off-by: hujianyang <hujianyang@huawei.com>
>> ---
>>  fs/namei.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/namei.c b/fs/namei.c
>> index a996bb4..0bc7734 100644
>> --- a/fs/namei.c
>> +++ b/fs/namei.c
>> @@ -2808,7 +2808,8 @@ looked_up:
>>  static int lookup_open(struct nameidata *nd, struct path *path,
>>                       struct file *file,
>>                       const struct open_flags *op,
>> -                     bool got_write, int *opened)
>> +                     bool got_write, int *opened,
>> +                     struct filename *name)
>>  {
>>       struct dentry *dir = nd->path.dentry;
>>       struct inode *dir_inode = dir->d_inode;
>> @@ -2854,6 +2855,9 @@ static int lookup_open(struct nameidata *nd, struct path *path,
>>                       error = -EROFS;
>>                       goto out_dput;
>>               }
>> +
>> +             audit_inode(name, dir, LOOKUP_PARENT);
>> +
>>               *opened |= FILE_CREATED;
>>               error = security_path_mknod(&nd->path, dentry, mode, 0);
>>               if (error)
>> @@ -2926,7 +2930,6 @@ static int do_last(struct nameidata *nd, struct path *path,
>>               if (error)
>>                       return error;
>>
>> -             audit_inode(name, dir, LOOKUP_PARENT);
>>               error = -EISDIR;
>>               /* trailing slashes? */
>>               if (nd->last.name[nd->last.len])
>> @@ -2945,7 +2948,7 @@ retry_lookup:
>>                */
>>       }
>>       mutex_lock(&dir->d_inode->i_mutex);
>> -     error = lookup_open(nd, path, file, op, got_write, opened);
>> +     error = lookup_open(nd, path, file, op, got_write, opened, name);
>>       mutex_unlock(&dir->d_inode->i_mutex);
>>
>>       if (error <= 0) {
>
> I'm not sure about this. Won't this cause us to miss creating audit
> records in some error conditions?
>
> For instance, if you end up not being able to create the file due to
> the fs being read-only, then I think this patch would make you miss the
> audit record for the parent.
>
> Might it be better to not plumb an extra pointer into lookup_open and
> just move the audit_inode calls around do_last in the appropriate
> places instead?
>
> --
> Jeff Layton <jlayton@primarydata.com>

Re: [PATCH] audit: vfs: fix audit records error when write to a file

[Richard Guy Briggs]

On 14/09/05, 胡剑阳 wrote:
> Hi Jeff,

Hello gentlemen,

> I don't know if we can keep audit_inode() in do_last(). I've tried
> this but found
> the only way to enable both write() and create() is moving it to lookup_open().
> I know this is not good, but I think we have no choice.
> 
> The current patch really miss the ROFS condition as you said. How about
> moving audit_inode() to the beginning of the O_CREAT case in lookup_open()?
> That's my mistake. After this, it seems no creating records will be missing.
> 
> I would like to change this and resend a new patch. I'm worry about if we miss
> something. Before I do this, do you have any suggestions? Or do you have a
> better way to keep audit_inode() in do_last()?

Could you add me to your Cc: list on this thread, please?  I'm
interested in the outcome...  Thanks!

> Thank you~!
> 
> Hu
> 
> 2014-09-05 18:50 GMT+08:00 Jeff Layton <jeff.layton@primarydata.com>:
> > On Fri, 5 Sep 2014 14:55:53 +0800
> > hujianyang <hujianyang@huawei.com> wrote:
> >
> >> commit 33e2208acfc1
> >>
> >> audit: vfs: fix audit_inode call in O_CREAT case of do_last
> >>
> >> fix a regression in auditing of open(..., O_CREAT) syscalls but
> >> import a new problem which lead the records of write operation
> >> confusion.
> >>
> >> This error can be reproduced by these steps:
> >>
> >>       touch /etc/test
> >>       echo "-w /etc/test" >>/etc/audit/audit.rules
> >>       /etc/init.d/auditd restart
> >>
> >>       echo "abc" >> /etc/test
> >>
> >> audit_name records are:
> >>
> >> type=PATH msg=audit(1409764556.196:67): item=0 name="/etc/" inode=5097 dev=00:01 mode=040755 ouid=0 ogid=0 rdev=00:00 nametype=PARENT
> >> type=PATH msg=audit(1409764556.196:67): item=1 name=(null) inode=23161 dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
> >> type=PATH msg=audit(1409764556.196:67): item=2 name=(null) inode=23161 dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
> >>
> >> but if we revert commit 33e2208acfc1, records are correct:
> >>
> >> type=PATH msg=audit(1409763058.192:219): item=0 name="/etc/test" inode=1275 dev=00:01 mode=0100644 ouid=0 ogid=0 rdev=00:00 nametype=NORMAL
> >>
> >> We shouldn't leave audit_inode(.., LOOKUP_PARENT) in O_CREAT case
> >> of do_last() because this branch don't really know if vfs need to
> >> create a new file. There is no need to do vfs_create() if we open
> >> an existing file with O_CREAT flag and write to it. But this
> >> audit_inode() in O_CREAT case will record a msg as we create a new
> >> file and confuse the records of write.
> >>
> >> This patch moves the audit for create operation to where a file
> >> really need to be created, the O_CREAT case in lookup_open().
> >> We have to add the pointer of struct filename as a parameter of
> >> lookup_open(). By doing this, the records of both create and write
> >> are correct.
> >>
> >> Signed-off-by: hujianyang <hujianyang@huawei.com>
> >> ---
> >>  fs/namei.c | 9 ++++++---
> >>  1 file changed, 6 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/fs/namei.c b/fs/namei.c
> >> index a996bb4..0bc7734 100644
> >> --- a/fs/namei.c
> >> +++ b/fs/namei.c
> >> @@ -2808,7 +2808,8 @@ looked_up:
> >>  static int lookup_open(struct nameidata *nd, struct path *path,
> >>                       struct file *file,
> >>                       const struct open_flags *op,
> >> -                     bool got_write, int *opened)
> >> +                     bool got_write, int *opened,
> >> +                     struct filename *name)
> >>  {
> >>       struct dentry *dir = nd->path.dentry;
> >>       struct inode *dir_inode = dir->d_inode;
> >> @@ -2854,6 +2855,9 @@ static int lookup_open(struct nameidata *nd, struct path *path,
> >>                       error = -EROFS;
> >>                       goto out_dput;
> >>               }
> >> +
> >> +             audit_inode(name, dir, LOOKUP_PARENT);
> >> +
> >>               *opened |= FILE_CREATED;
> >>               error = security_path_mknod(&nd->path, dentry, mode, 0);
> >>               if (error)
> >> @@ -2926,7 +2930,6 @@ static int do_last(struct nameidata *nd, struct path *path,
> >>               if (error)
> >>                       return error;
> >>
> >> -             audit_inode(name, dir, LOOKUP_PARENT);
> >>               error = -EISDIR;
> >>               /* trailing slashes? */
> >>               if (nd->last.name[nd->last.len])
> >> @@ -2945,7 +2948,7 @@ retry_lookup:
> >>                */
> >>       }
> >>       mutex_lock(&dir->d_inode->i_mutex);
> >> -     error = lookup_open(nd, path, file, op, got_write, opened);
> >> +     error = lookup_open(nd, path, file, op, got_write, opened, name);
> >>       mutex_unlock(&dir->d_inode->i_mutex);
> >>
> >>       if (error <= 0) {
> >
> > I'm not sure about this. Won't this cause us to miss creating audit
> > records in some error conditions?
> >
> > For instance, if you end up not being able to create the file due to
> > the fs being read-only, then I think this patch would make you miss the
> > audit record for the parent.
> >
> > Might it be better to not plumb an extra pointer into lookup_open and
> > just move the audit_inode calls around do_last in the appropriate
> > places instead?
> >
> > --
> > Jeff Layton <jlayton@primarydata.com>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545