diff mbox

[RFC,ghak32,V2,13/13] debug audit: read container ID of a process

Message ID 1081821010c124fe4e35984ec3dac1654453bb7c.1521179281.git.rgb@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Richard Guy Briggs March 16, 2018, 9 a.m. UTC
Add support for reading the container ID from the proc filesystem.

This is a read from the proc entry of the form /proc/PID/containerid
where PID is the process ID of the task whose container ID is sought.

The read expects up to a u64 value (unset: 18446744073709551615).

Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 fs/proc/base.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

Comments

Steve Grubb May 21, 2018, 7:16 p.m. UTC | #1
On Friday, March 16, 2018 5:00:40 AM EDT Richard Guy Briggs wrote:
> Add support for reading the container ID from the proc filesystem.

I think this could be useful in general. Please consider this to be part of 
the full patch set and not something merely used to debug the patches.

-Steve

> This is a read from the proc entry of the form /proc/PID/containerid
> where PID is the process ID of the task whose container ID is sought.
> 
> The read expects up to a u64 value (unset: 18446744073709551615).
> 
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  fs/proc/base.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 6ce4fbe..f66d1e2 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -1300,6 +1300,21 @@ static ssize_t proc_sessionid_read(struct file *
> file, char __user * buf, .llseek		= generic_file_llseek,
>  };
> 
> +static ssize_t proc_containerid_read(struct file *file, char __user *buf,
> +				  size_t count, loff_t *ppos)
> +{
> +	struct inode *inode = file_inode(file);
> +	struct task_struct *task = get_proc_task(inode);
> +	ssize_t length;
> +	char tmpbuf[TMPBUFLEN*2];
> +
> +	if (!task)
> +		return -ESRCH;
> +	length = scnprintf(tmpbuf, TMPBUFLEN*2, "%llu",
> audit_get_containerid(task)); +	put_task_struct(task);
> +	return simple_read_from_buffer(buf, count, ppos, tmpbuf, length);
> +}
> +
>  static ssize_t proc_containerid_write(struct file *file, const char __user
> *buf, size_t count, loff_t *ppos)
>  {
> @@ -1330,6 +1345,7 @@ static ssize_t proc_containerid_write(struct file
> *file, const char __user *buf, }
> 
>  static const struct file_operations proc_containerid_operations = {
> +	.read		= proc_containerid_read,
>  	.write		= proc_containerid_write,
>  	.llseek		= generic_file_llseek,
>  };
> @@ -2996,7 +3012,7 @@ static int proc_pid_patch_state(struct seq_file *m,
> struct pid_namespace *ns, #ifdef CONFIG_AUDITSYSCALL
>  	REG("loginuid",   S_IWUSR|S_IRUGO, proc_loginuid_operations),
>  	REG("sessionid",  S_IRUGO, proc_sessionid_operations),
> -	REG("containerid", S_IWUSR, proc_containerid_operations),
> +	REG("containerid", S_IWUSR|S_IRUSR, proc_containerid_operations),
>  #endif
>  #ifdef CONFIG_FAULT_INJECTION
>  	REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
> @@ -3391,7 +3407,7 @@ static int proc_tid_comm_permission(struct inode
> *inode, int mask) #ifdef CONFIG_AUDITSYSCALL
>  	REG("loginuid",  S_IWUSR|S_IRUGO, proc_loginuid_operations),
>  	REG("sessionid",  S_IRUGO, proc_sessionid_operations),
> -	REG("containerid", S_IWUSR, proc_containerid_operations),
> +	REG("containerid", S_IWUSR|S_IRUSR, proc_containerid_operations),
>  #endif
>  #ifdef CONFIG_FAULT_INJECTION
>  	REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
Eric W. Biederman May 21, 2018, 7:19 p.m. UTC | #2
Steve Grubb <sgrubb@redhat.com> writes:

> On Friday, March 16, 2018 5:00:40 AM EDT Richard Guy Briggs wrote:
>> Add support for reading the container ID from the proc filesystem.
>
> I think this could be useful in general. Please consider this to be part of 
> the full patch set and not something merely used to debug the patches.

Only with an audit specific name.

As it is:

Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>

The truth is the containerid name really stinks and is quite confusing
and does not imply that the label applies only to audit.  And little
things like this make me extremely uncofortable with it.

Eric
Paul Moore May 21, 2018, 8:06 p.m. UTC | #3
On Mon, May 21, 2018 at 3:19 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Steve Grubb <sgrubb@redhat.com> writes:
>
>> On Friday, March 16, 2018 5:00:40 AM EDT Richard Guy Briggs wrote:
>>> Add support for reading the container ID from the proc filesystem.
>>
>> I think this could be useful in general. Please consider this to be part of
>> the full patch set and not something merely used to debug the patches.
>
> Only with an audit specific name.
>
> As it is:
>
> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
> The truth is the containerid name really stinks and is quite confusing
> and does not imply that the label applies only to audit.  And little
> things like this make me extremely uncofortable with it.

It also makes the audit container ID (notice how I *always* call it
the *audit* container ID? that is not an accident) available for
userspace applications to abuse.  Perhaps in the future we can look at
ways to make this more available to applications, but this patch is
not the answer.
Richard Guy Briggs May 22, 2018, 5:35 p.m. UTC | #4
On 2018-05-21 16:06, Paul Moore wrote:
> On Mon, May 21, 2018 at 3:19 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
> > Steve Grubb <sgrubb@redhat.com> writes:
> >> On Friday, March 16, 2018 5:00:40 AM EDT Richard Guy Briggs wrote:
> >>> Add support for reading the container ID from the proc filesystem.
> >>
> >> I think this could be useful in general. Please consider this to be part of
> >> the full patch set and not something merely used to debug the patches.
> >
> > Only with an audit specific name.
> >
> > As it is:
> >
> > Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> >
> > The truth is the containerid name really stinks and is quite confusing
> > and does not imply that the label applies only to audit.  And little
> > things like this make me extremely uncofortable with it.
> 
> It also makes the audit container ID (notice how I *always* call it
> the *audit* container ID? that is not an accident) available for
> userspace applications to abuse.  Perhaps in the future we can look at
> ways to make this more available to applications, but this patch is
> not the answer.

Do you have a productive suggestion?

> paul moore

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
Paul Moore May 22, 2018, 6:59 p.m. UTC | #5
On Tue, May 22, 2018 at 1:35 PM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-05-21 16:06, Paul Moore wrote:
>> On Mon, May 21, 2018 at 3:19 PM, Eric W. Biederman <ebiederm@xmission.com> wrote:
>> > Steve Grubb <sgrubb@redhat.com> writes:
>> >> On Friday, March 16, 2018 5:00:40 AM EDT Richard Guy Briggs wrote:
>> >>> Add support for reading the container ID from the proc filesystem.
>> >>
>> >> I think this could be useful in general. Please consider this to be part of
>> >> the full patch set and not something merely used to debug the patches.
>> >
>> > Only with an audit specific name.
>> >
>> > As it is:
>> >
>> > Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>> >
>> > The truth is the containerid name really stinks and is quite confusing
>> > and does not imply that the label applies only to audit.  And little
>> > things like this make me extremely uncofortable with it.
>>
>> It also makes the audit container ID (notice how I *always* call it
>> the *audit* container ID? that is not an accident) available for
>> userspace applications to abuse.  Perhaps in the future we can look at
>> ways to make this more available to applications, but this patch is
>> not the answer.
>
> Do you have a productive suggestion?

I haven't given it much thought beyond our discussions and until we
get the basic audit container ID support in place (all the other parts
of this patchset) I doubt I'll be giving it much thought.
diff mbox

Patch

diff --git a/fs/proc/base.c b/fs/proc/base.c
index 6ce4fbe..f66d1e2 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1300,6 +1300,21 @@  static ssize_t proc_sessionid_read(struct file * file, char __user * buf,
 	.llseek		= generic_file_llseek,
 };
 
+static ssize_t proc_containerid_read(struct file *file, char __user *buf,
+				  size_t count, loff_t *ppos)
+{
+	struct inode *inode = file_inode(file);
+	struct task_struct *task = get_proc_task(inode);
+	ssize_t length;
+	char tmpbuf[TMPBUFLEN*2];
+
+	if (!task)
+		return -ESRCH;
+	length = scnprintf(tmpbuf, TMPBUFLEN*2, "%llu", audit_get_containerid(task));
+	put_task_struct(task);
+	return simple_read_from_buffer(buf, count, ppos, tmpbuf, length);
+}
+
 static ssize_t proc_containerid_write(struct file *file, const char __user *buf,
 				   size_t count, loff_t *ppos)
 {
@@ -1330,6 +1345,7 @@  static ssize_t proc_containerid_write(struct file *file, const char __user *buf,
 }
 
 static const struct file_operations proc_containerid_operations = {
+	.read		= proc_containerid_read,
 	.write		= proc_containerid_write,
 	.llseek		= generic_file_llseek,
 };
@@ -2996,7 +3012,7 @@  static int proc_pid_patch_state(struct seq_file *m, struct pid_namespace *ns,
 #ifdef CONFIG_AUDITSYSCALL
 	REG("loginuid",   S_IWUSR|S_IRUGO, proc_loginuid_operations),
 	REG("sessionid",  S_IRUGO, proc_sessionid_operations),
-	REG("containerid", S_IWUSR, proc_containerid_operations),
+	REG("containerid", S_IWUSR|S_IRUSR, proc_containerid_operations),
 #endif
 #ifdef CONFIG_FAULT_INJECTION
 	REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
@@ -3391,7 +3407,7 @@  static int proc_tid_comm_permission(struct inode *inode, int mask)
 #ifdef CONFIG_AUDITSYSCALL
 	REG("loginuid",  S_IWUSR|S_IRUGO, proc_loginuid_operations),
 	REG("sessionid",  S_IRUGO, proc_sessionid_operations),
-	REG("containerid", S_IWUSR, proc_containerid_operations),
+	REG("containerid", S_IWUSR|S_IRUSR, proc_containerid_operations),
 #endif
 #ifdef CONFIG_FAULT_INJECTION
 	REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),