* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Jiri Kosina @ 2019-01-05 23:09 UTC (permalink / raw)
To: Jann Horn
Cc: Linus Torvalds, Andrew Morton, Greg KH, Peter Zijlstra,
Michal Hocko, Linux-MM, kernel list, Linux API
In-Reply-To: <CAG48ez2jAp9xkPXQmVXm0PqNrFGscg9BufQRem2UD8FGX-YzPw@mail.gmail.com>
On Sat, 5 Jan 2019, Jann Horn wrote:
> > Provide vm.mincore_privileged sysctl, which makes it possible to mincore()
> > start returning -EPERM in case it's invoked by a process lacking
> > CAP_SYS_ADMIN.
> >
> > The default behavior stays "mincore() can be used by anybody" in order to
> > be conservative with respect to userspace behavior.
> >
> > [1] https://www.theregister.co.uk/2019/01/05/boffins_beat_page_cache/
>
> Just checking: I guess /proc/$pid/pagemap (iow, the pagemap_read()
> handler) is less problematic because it only returns data about the
> state of page tables, and doesn't query the address_space? In other
> words, it permits monitoring evictions, but non-intrusively detecting
> that something has been loaded into memory by another process is
> harder?
So I was just about to immediately reply that we don't expose pagemap
anymore due to rowhammer, but apparently that's not true any more
(this behavioud was originally introduced by ab676b7d6fbf, but then
changed via 1c90308e7a77 (and further adjusted for swap entries in
ab6ecf247a, but I guess that's not all that interesting).
Hmm.
But unless it has been mapped with MAP_POPULATE (whcih is outside the
attacker's control), there is no guarantee that the mappings would
actually be there, right?
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Linus Torvalds @ 2019-01-05 23:05 UTC (permalink / raw)
To: Jann Horn
Cc: Jiri Kosina, Andrew Morton, Greg KH, Peter Zijlstra, Michal Hocko,
Linux-MM, kernel list, Linux API
In-Reply-To: <CAG48ez2jAp9xkPXQmVXm0PqNrFGscg9BufQRem2UD8FGX-YzPw@mail.gmail.com>
On Sat, Jan 5, 2019 at 2:55 PM Jann Horn <jannh@google.com> wrote:
>
> Just checking: I guess /proc/$pid/pagemap (iow, the pagemap_read()
> handler) is less problematic because it only returns data about the
> state of page tables, and doesn't query the address_space? In other
> words, it permits monitoring evictions, but non-intrusively detecting
> that something has been loaded into memory by another process is
> harder?
Yes. I think it was brought up during the original report, but to use
the pagemap for this, you'd basically need to first populate all the
pages, and then wait for pageout.
So pagemap *does* leak very similar data, but it's much harder to use
as an attack vector.
That said, I think "mincore()" is just the simple one. You *can* (and
this was also discussed on the security list) do things like
- fault in a single page
- the kernel will opportunistically fault in pages that it already
has available _around_ that page.
- then use pagemap (or just _timing_ - no real kernel support needed)
to see if those pages are now mapped in your address space
so honestly, mincore is just the "big hammer" and easy way to get at
some of this data. But it's probably worth closing exactly because
it's easy. There are other ways to get at the "are these pages mapped"
information, but they are a lot more combersome to use.
Side note: maybe we could just remove the "__mincore_unmapped_range()"
thing entirely, and basically make mincore() do what pagemap does,
which is to say "are the pages mapped in this VM".
That would be nicer than my patch, simply because removing code is
always nice. And arguably it's a better semantic anyway.
Linus
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Jann Horn @ 2019-01-05 22:54 UTC (permalink / raw)
To: Jiri Kosina
Cc: Linus Torvalds, Andrew Morton, Greg KH, Peter Zijlstra,
Michal Hocko, Linux-MM, kernel list, Linux API
In-Reply-To: <nycvar.YFH.7.76.1901051817390.16954@cbobk.fhfr.pm>
On Sat, Jan 5, 2019 at 6:27 PM Jiri Kosina <jikos@kernel.org> wrote:
> There are possibilities [1] how mincore() could be used as a converyor of
> a sidechannel information about pagecache metadata.
>
> Provide vm.mincore_privileged sysctl, which makes it possible to mincore()
> start returning -EPERM in case it's invoked by a process lacking
> CAP_SYS_ADMIN.
>
> The default behavior stays "mincore() can be used by anybody" in order to
> be conservative with respect to userspace behavior.
>
> [1] https://www.theregister.co.uk/2019/01/05/boffins_beat_page_cache/
Just checking: I guess /proc/$pid/pagemap (iow, the pagemap_read()
handler) is less problematic because it only returns data about the
state of page tables, and doesn't query the address_space? In other
words, it permits monitoring evictions, but non-intrusively detecting
that something has been loaded into memory by another process is
harder?
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Linus Torvalds @ 2019-01-05 21:54 UTC (permalink / raw)
To: Jiri Kosina, Masatake YAMATO
Cc: Andrew Morton, Greg KH, Peter Zijlstra, Michal Hocko, linux-mm,
Linux List Kernel Mailing, linux-api
In-Reply-To: <nycvar.YFH.7.76.1901052131480.16954@cbobk.fhfr.pm>
On Sat, Jan 5, 2019 at 12:43 PM Jiri Kosina <jikos@kernel.org> wrote:
>
> > Who actually _uses_ mincore()? That's probably the best guide to what
> > we should do. Maybe they open the file read-only even if they are the
> > owner, and we really should look at file ownership instead.
>
> Yeah, well
>
> https://codesearch.debian.net/search?q=mincore
>
> is a bit too much mess to get some idea quickly I am afraid.
Yeah, heh.
And the first hit is 'fincore', which probably nobody cares about
anyway, but it does
fd = open (name, O_RDONLY)
..
mmap(window, len, PROT_NONE, MAP_PRIVATE, ..
so if we want to keep that working, we'd really need to actually check
file ownership rather than just looking at f_mode.
But I don't know if anybody *uses* and cares about fincore, and it's
particularly questionable for non-root users.
And the Android go runtime code seems to oddly use mincore to figure
out page size:
// try using mincore to detect the physical page size.
// mincore should return EINVAL when address is not a multiple of
system page size.
which is all kinds of odd, but whatever.. Why mincore, rather than
something sane and obvious like mmap? Don't ask me...
Anyway, the Debian code search just results in mostly non-present
stuff. It's sad that google code search is no more. It was great for
exactly these kinds of questions.
The mono runtime seems to have some mono_pages_not_faulted() function,
but I don't know if people use it for file mappings, and I couldn't
find any interesting users of it.
I didn't find anything that seems to really care, but I gave up after
a few pages of really boring stuff.
Linus
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Jiri Kosina @ 2019-01-05 20:43 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andrew Morton, Greg KH, Peter Zijlstra, Michal Hocko, linux-mm,
Linux List Kernel Mailing, linux-api
In-Reply-To: <CAHk-=whGmE4QVr6NbgHnrVGVENfM3s1y6GNbsfh8PcOg=6bpqw@mail.gmail.com>
On Sat, 5 Jan 2019, Linus Torvalds wrote:
> > I am still not completely sure what to return in such cases though; we can
> > either blatantly lie and always pretend that the pages are resident
>
> That's what my untested patch did. Or maybe just claim they are all
> not present?
Thinking about it a little bit more, I believe Vlastimil has a good point
with 'non present' potentially causing more bogus activity in userspace in
response (in an effort to actually make them present, and failing
indefinitely).
IOW, I think it's a reasonable expectation that the common scenario is
"check if it's present, and if not, try to fault it in" instead of "check
if it's present, and if it is, try to evict it".
> And again, that patch was entirely untested, so it may be garbage and
> have some fundamental problem.
I will be travelling for next ~24 hours, but I have just asked our QA guys
to run it through some basic battery of testing (which will probably
happen on monday anyway).
> I also don't know exactly what rule might make most sense, but "you can
> write to the file" certainly to me implies that you also could know what
> parts of it are in-core.
I think it's reasonable; I can't really imagine any sidechannel to a
global state be possibly mounted on valid R/W mappings. I'd guess that
probably the most interesting here are the code segments of shared
libraries, allowing to trace victim's execution.
> Who actually _uses_ mincore()? That's probably the best guide to what
> we should do. Maybe they open the file read-only even if they are the
> owner, and we really should look at file ownership instead.
Yeah, well
https://codesearch.debian.net/search?q=mincore
is a bit too much mess to get some idea quickly I am afraid.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Linus Torvalds @ 2019-01-05 20:17 UTC (permalink / raw)
To: Jiri Kosina
Cc: Andrew Morton, Greg KH, Peter Zijlstra, Michal Hocko, linux-mm,
Linux List Kernel Mailing, linux-api
In-Reply-To: <nycvar.YFH.7.76.1901052108390.16954@cbobk.fhfr.pm>
[ Crossed emails ]
On Sat, Jan 5, 2019 at 12:12 PM Jiri Kosina <jikos@kernel.org> wrote:
>
> I am still not completely sure what to return in such cases though; we can
> either blatantly lie and always pretend that the pages are resident
That's what my untested patch did. Or maybe just claim they are all
not present?
And again, that patch was entirely untested, so it may be garbage and
have some fundamental problem. I also don't know exactly what rule
might make most sense, but "you can write to the file" certainly to me
implies that you also could know what parts of it are in-core.
Who actually _uses_ mincore()? That's probably the best guide to what
we should do. Maybe they open the file read-only even if they are the
owner, and we really should look at file ownership instead.
I tried to make that "can_do_mincore()" function easy to understand
and easy to just modify to some sane state.
Again, my patch is meant as a "perhaps something like this?" rather
than some "this is _exactly_ how it must be done". Take the patch as a
quick suggestion, not some final answer.
Linus
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Linus Torvalds @ 2019-01-05 20:13 UTC (permalink / raw)
To: Jiri Kosina
Cc: Andrew Morton, Greg KH, Peter Zijlstra, Michal Hocko, linux-mm,
Linux List Kernel Mailing, linux-api
In-Reply-To: <CAHk-=wicks2BEwm1BhdvEj_P3yawmvQuG3NOnjhdrUDEtTGizw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 335 bytes --]
On Sat, Jan 5, 2019 at 11:46 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Can we please just limit it to vma's that are either anonymous, or map
> a file that the user actually owns?
.. or slightly simpler: a file that the user opened for writing.
IOW, some (TOTALLY UNTESTED!) patch like this?
Linus
[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1161 bytes --]
mm/mincore.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/mm/mincore.c b/mm/mincore.c
index 218099b5ed31..61e38895fb02 100644
--- a/mm/mincore.c
+++ b/mm/mincore.c
@@ -169,6 +169,13 @@ static int mincore_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
return 0;
}
+static inline bool can_do_mincore(struct vm_area_struct *vma)
+{
+ return vma_is_anonymous(vma)
+ || (vma->vm_file && (vma->vm_file->f_mode & FMODE_WRITE))
+ || capable(CAP_SYS_ADMIN);
+}
+
/*
* Do a chunk of "sys_mincore()". We've already checked
* all the arguments, we hold the mmap semaphore: we should
@@ -189,8 +196,13 @@ static long do_mincore(unsigned long addr, unsigned long pages, unsigned char *v
vma = find_vma(current->mm, addr);
if (!vma || addr < vma->vm_start)
return -ENOMEM;
- mincore_walk.mm = vma->vm_mm;
end = min(vma->vm_end, addr + (pages << PAGE_SHIFT));
+ if (!can_do_mincore(vma)) {
+ unsigned long pages = (end - addr) >> PAGE_SHIFT;
+ memset(vec, 1, pages);
+ return pages;
+ }
+ mincore_walk.mm = vma->vm_mm;
err = walk_page_range(addr, end, &mincore_walk);
if (err < 0)
return err;
^ permalink raw reply related
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Jiri Kosina @ 2019-01-05 20:12 UTC (permalink / raw)
To: Linus Torvalds
Cc: Andrew Morton, Greg KH, Peter Zijlstra, Michal Hocko, linux-mm,
Linux List Kernel Mailing, linux-api
In-Reply-To: <CAHk-=wicks2BEwm1BhdvEj_P3yawmvQuG3NOnjhdrUDEtTGizw@mail.gmail.com>
On Sat, 5 Jan 2019, Linus Torvalds wrote:
> > There are possibilities [1] how mincore() could be used as a converyor of
> > a sidechannel information about pagecache metadata.
>
> Can we please just limit it to vma's that are either anonymous, or map
> a file that the user actually owns?
>
> Then the capability check could be for "override the file owner check"
> instead, which makes tons of sense.
Makes sense.
I am still not completely sure what to return in such cases though; we can
either blatantly lie and always pretend that the pages are resident (to
avoid calling process entering some prefaulting mode), or return -ENOMEM
for mappings of files that don't belong to the user (in case it's not
CAP_SYS_ADMIN one).
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: kbuild test robot @ 2019-01-05 19:56 UTC (permalink / raw)
To: Jiri Kosina
Cc: kbuild-all, Linus Torvalds, Andrew Morton, Greg KH,
Peter Zijlstra, Michal Hocko, linux-mm, linux-kernel, linux-api
In-Reply-To: <nycvar.YFH.7.76.1901051817390.16954@cbobk.fhfr.pm>
[-- Attachment #1: Type: text/plain, Size: 1018 bytes --]
Hi Jiri,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.20 next-20190103]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Jiri-Kosina/mm-mincore-allow-for-making-sys_mincore-privileged/20190106-013707
config: microblaze-nommu_defconfig (attached as .config)
compiler: microblaze-linux-gcc (GCC) 8.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.1.0 make.cross ARCH=microblaze
All errors (new ones prefixed by >>):
>> kernel/sysctl.o:(.data+0x67c): undefined reference to `sysctl_mincore_privileged'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 12566 bytes --]
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Linus Torvalds @ 2019-01-05 19:46 UTC (permalink / raw)
To: Jiri Kosina
Cc: Andrew Morton, Greg KH, Peter Zijlstra, Michal Hocko, linux-mm,
Linux List Kernel Mailing, linux-api
In-Reply-To: <nycvar.YFH.7.76.1901051817390.16954@cbobk.fhfr.pm>
On Sat, Jan 5, 2019 at 9:27 AM Jiri Kosina <jikos@kernel.org> wrote:
>
> From: Jiri Kosina <jkosina@suse.cz>
>
> There are possibilities [1] how mincore() could be used as a converyor of
> a sidechannel information about pagecache metadata.
Can we please just limit it to vma's that are either anonymous, or map
a file that the user actually owns?
Then the capability check could be for "override the file owner check"
instead, which makes tons of sense.
No new sysctl's for something like this, please.
Linus
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: kbuild test robot @ 2019-01-05 19:44 UTC (permalink / raw)
To: Jiri Kosina
Cc: kbuild-all, Linus Torvalds, Andrew Morton, Greg KH,
Peter Zijlstra, Michal Hocko, linux-mm, linux-kernel, linux-api
In-Reply-To: <nycvar.YFH.7.76.1901051817390.16954@cbobk.fhfr.pm>
[-- Attachment #1: Type: text/plain, Size: 1001 bytes --]
Hi Jiri,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.20 next-20190103]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Jiri-Kosina/mm-mincore-allow-for-making-sys_mincore-privileged/20190106-013707
config: c6x-evmc6678_defconfig (attached as .config)
compiler: c6x-elf-gcc (GCC) 8.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.1.0 make.cross ARCH=c6x
All errors (new ones prefixed by >>):
>> kernel/sysctl.o:(.fardata+0x6a0): undefined reference to `sysctl_mincore_privileged'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 5258 bytes --]
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Vlastimil Babka @ 2019-01-05 19:38 UTC (permalink / raw)
To: Jiri Kosina
Cc: Linus Torvalds, Andrew Morton, Greg KH, Peter Zijlstra,
Michal Hocko, linux-mm, linux-kernel, linux-api
In-Reply-To: <nycvar.YFH.7.76.1901052016250.16954@cbobk.fhfr.pm>
On 5.1.2019 20:24, Jiri Kosina wrote:
> On Sat, 5 Jan 2019, Vlastimil Babka wrote:
>
>>> There are possibilities [1] how mincore() could be used as a converyor of
>>> a sidechannel information about pagecache metadata.
>>>
>>> Provide vm.mincore_privileged sysctl, which makes it possible to mincore()
>>> start returning -EPERM in case it's invoked by a process lacking
>>> CAP_SYS_ADMIN.
>>
>> Haven't checked the details yet, but wouldn't it be safe if anonymous private
>> mincore() kept working, and restrictions were applied only to page cache?
>
> I was considering that, but then I decided not to do so, as that'd make
> the interface even more confusing and semantics non-obvious in the
> 'privileged' case.
>
>>> The default behavior stays "mincore() can be used by anybody" in order to
>>> be conservative with respect to userspace behavior.
>>
>> What if we lied instead of returned -EPERM, to not break userspace so
>> obviously? I guess false positive would be the safer lie?
>
> So your proposal basically would be
>
> if (privileged && !CAP_SYS_ADMIN)
> if (pagecache)
> return false;
I was thinking about "return true" here, assuming that userspace generally wants
to ensure itself there won't be page faults when it starts doing something
critical, and if it sees a "false" it will try to do some kind of prefaulting,
possibly in a loop. There might be somebody trying to make sure something is out
of pagecache (it wants to see "false"), but can't think of anything except
benchmarks?
> else
> return do_mincore()
>
> right ?
>
> I think userspace would hate us for that semantics, but on the other hand
> I can sort of understand the 'mincore() is racy anyway, so what' argument,
> if that's what you are suggesting.
>
> But then, I have no idea what userspace is using mincore() for.
> https://codesearch.debian.net/search?q=mincore might provide some insight
> I guess (thanks Matthew).
>
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Jiri Kosina @ 2019-01-05 19:24 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Linus Torvalds, Andrew Morton, Greg KH, Peter Zijlstra,
Michal Hocko, linux-mm, linux-kernel, linux-api
In-Reply-To: <d4846cb2-2a4b-b8b3-daac-e5f51751bbf1@suse.cz>
On Sat, 5 Jan 2019, Vlastimil Babka wrote:
> > There are possibilities [1] how mincore() could be used as a converyor of
> > a sidechannel information about pagecache metadata.
> >
> > Provide vm.mincore_privileged sysctl, which makes it possible to mincore()
> > start returning -EPERM in case it's invoked by a process lacking
> > CAP_SYS_ADMIN.
>
> Haven't checked the details yet, but wouldn't it be safe if anonymous private
> mincore() kept working, and restrictions were applied only to page cache?
I was considering that, but then I decided not to do so, as that'd make
the interface even more confusing and semantics non-obvious in the
'privileged' case.
> > The default behavior stays "mincore() can be used by anybody" in order to
> > be conservative with respect to userspace behavior.
>
> What if we lied instead of returned -EPERM, to not break userspace so
> obviously? I guess false positive would be the safer lie?
So your proposal basically would be
if (privileged && !CAP_SYS_ADMIN)
if (pagecache)
return false;
else
return do_mincore()
right ?
I think userspace would hate us for that semantics, but on the other hand
I can sort of understand the 'mincore() is racy anyway, so what' argument,
if that's what you are suggesting.
But then, I have no idea what userspace is using mincore() for.
https://codesearch.debian.net/search?q=mincore might provide some insight
I guess (thanks Matthew).
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Vlastimil Babka @ 2019-01-05 19:14 UTC (permalink / raw)
To: Jiri Kosina, Linus Torvalds, Andrew Morton, Greg KH,
Peter Zijlstra, Michal Hocko
Cc: linux-mm, linux-kernel, linux-api
In-Reply-To: <nycvar.YFH.7.76.1901051817390.16954@cbobk.fhfr.pm>
On 5.1.2019 18:27, Jiri Kosina wrote:
> From: Jiri Kosina <jkosina@suse.cz>
>
> There are possibilities [1] how mincore() could be used as a converyor of
> a sidechannel information about pagecache metadata.
>
> Provide vm.mincore_privileged sysctl, which makes it possible to mincore()
> start returning -EPERM in case it's invoked by a process lacking
> CAP_SYS_ADMIN.
Haven't checked the details yet, but wouldn't it be safe if anonymous private
mincore() kept working, and restrictions were applied only to page cache?
> The default behavior stays "mincore() can be used by anybody" in order to
> be conservative with respect to userspace behavior.
What if we lied instead of returned -EPERM, to not break userspace so obviously?
I guess false positive would be the safer lie?
> [1] https://www.theregister.co.uk/2019/01/05/boffins_beat_page_cache/
>
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
> ---
> Documentation/sysctl/vm.txt | 9 +++++++++
> kernel/sysctl.c | 8 ++++++++
> mm/mincore.c | 5 +++++
> 3 files changed, 22 insertions(+)
>
> diff --git a/Documentation/sysctl/vm.txt b/Documentation/sysctl/vm.txt
> index 187ce4f599a2..afb8635e925e 100644
> --- a/Documentation/sysctl/vm.txt
> +++ b/Documentation/sysctl/vm.txt
> @@ -41,6 +41,7 @@ Currently, these files are in /proc/sys/vm:
> - min_free_kbytes
> - min_slab_ratio
> - min_unmapped_ratio
> +- mincore_privileged
> - mmap_min_addr
> - mmap_rnd_bits
> - mmap_rnd_compat_bits
> @@ -485,6 +486,14 @@ files and similar are considered.
> The default is 1 percent.
>
> ==============================================================
> +mincore_privileged:
> +
> +mincore() could be potentially used to mount a side-channel attack against
> +pagecache metadata. This sysctl provides system administrators means to
> +make it available only to processess that own CAP_SYS_ADMIN capability.
> +
> +The default is 0, which means mincore() can be used without restrictions.
> +==============================================================
>
> mmap_min_addr
>
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 1825f712e73b..f03cb07c8dd4 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -114,6 +114,7 @@ extern unsigned int sysctl_nr_open_min, sysctl_nr_open_max;
> #ifndef CONFIG_MMU
> extern int sysctl_nr_trim_pages;
> #endif
> +extern int sysctl_mincore_privileged;
>
> /* Constants used for minimum and maximum */
> #ifdef CONFIG_LOCKUP_DETECTOR
> @@ -1684,6 +1685,13 @@ static struct ctl_table vm_table[] = {
> .extra2 = (void *)&mmap_rnd_compat_bits_max,
> },
> #endif
> + {
> + .procname = "mincore_privileged",
> + .data = &sysctl_mincore_privileged,
> + .maxlen = sizeof(sysctl_mincore_privileged),
> + .mode = 0644,
> + .proc_handler = proc_dointvec,
> + },
> { }
> };
>
> diff --git a/mm/mincore.c b/mm/mincore.c
> index 218099b5ed31..77d4928cdfaa 100644
> --- a/mm/mincore.c
> +++ b/mm/mincore.c
> @@ -21,6 +21,8 @@
> #include <linux/uaccess.h>
> #include <asm/pgtable.h>
>
> +int sysctl_mincore_privileged;
> +
> static int mincore_hugetlb(pte_t *pte, unsigned long hmask, unsigned long addr,
> unsigned long end, struct mm_walk *walk)
> {
> @@ -228,6 +230,9 @@ SYSCALL_DEFINE3(mincore, unsigned long, start, size_t, len,
> unsigned long pages;
> unsigned char *tmp;
>
> + if (sysctl_mincore_privileged && !capable(CAP_SYS_ADMIN))
> + return -EPERM;
> +
> /* Check the start address: needs to be page-aligned.. */
> if (start & ~PAGE_MASK)
> return -EINVAL;
>
^ permalink raw reply
* [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Jiri Kosina @ 2019-01-05 17:27 UTC (permalink / raw)
To: Linus Torvalds, Andrew Morton, Greg KH, Peter Zijlstra,
Michal Hocko
Cc: linux-mm, linux-kernel, linux-api
From: Jiri Kosina <jkosina@suse.cz>
There are possibilities [1] how mincore() could be used as a converyor of
a sidechannel information about pagecache metadata.
Provide vm.mincore_privileged sysctl, which makes it possible to mincore()
start returning -EPERM in case it's invoked by a process lacking
CAP_SYS_ADMIN.
The default behavior stays "mincore() can be used by anybody" in order to
be conservative with respect to userspace behavior.
[1] https://www.theregister.co.uk/2019/01/05/boffins_beat_page_cache/
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
---
Documentation/sysctl/vm.txt | 9 +++++++++
kernel/sysctl.c | 8 ++++++++
mm/mincore.c | 5 +++++
3 files changed, 22 insertions(+)
diff --git a/Documentation/sysctl/vm.txt b/Documentation/sysctl/vm.txt
index 187ce4f599a2..afb8635e925e 100644
--- a/Documentation/sysctl/vm.txt
+++ b/Documentation/sysctl/vm.txt
@@ -41,6 +41,7 @@ Currently, these files are in /proc/sys/vm:
- min_free_kbytes
- min_slab_ratio
- min_unmapped_ratio
+- mincore_privileged
- mmap_min_addr
- mmap_rnd_bits
- mmap_rnd_compat_bits
@@ -485,6 +486,14 @@ files and similar are considered.
The default is 1 percent.
==============================================================
+mincore_privileged:
+
+mincore() could be potentially used to mount a side-channel attack against
+pagecache metadata. This sysctl provides system administrators means to
+make it available only to processess that own CAP_SYS_ADMIN capability.
+
+The default is 0, which means mincore() can be used without restrictions.
+==============================================================
mmap_min_addr
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 1825f712e73b..f03cb07c8dd4 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -114,6 +114,7 @@ extern unsigned int sysctl_nr_open_min, sysctl_nr_open_max;
#ifndef CONFIG_MMU
extern int sysctl_nr_trim_pages;
#endif
+extern int sysctl_mincore_privileged;
/* Constants used for minimum and maximum */
#ifdef CONFIG_LOCKUP_DETECTOR
@@ -1684,6 +1685,13 @@ static struct ctl_table vm_table[] = {
.extra2 = (void *)&mmap_rnd_compat_bits_max,
},
#endif
+ {
+ .procname = "mincore_privileged",
+ .data = &sysctl_mincore_privileged,
+ .maxlen = sizeof(sysctl_mincore_privileged),
+ .mode = 0644,
+ .proc_handler = proc_dointvec,
+ },
{ }
};
diff --git a/mm/mincore.c b/mm/mincore.c
index 218099b5ed31..77d4928cdfaa 100644
--- a/mm/mincore.c
+++ b/mm/mincore.c
@@ -21,6 +21,8 @@
#include <linux/uaccess.h>
#include <asm/pgtable.h>
+int sysctl_mincore_privileged;
+
static int mincore_hugetlb(pte_t *pte, unsigned long hmask, unsigned long addr,
unsigned long end, struct mm_walk *walk)
{
@@ -228,6 +230,9 @@ SYSCALL_DEFINE3(mincore, unsigned long, start, size_t, len,
unsigned long pages;
unsigned char *tmp;
+ if (sysctl_mincore_privileged && !capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
/* Check the start address: needs to be page-aligned.. */
if (start & ~PAGE_MASK)
return -EINVAL;
--
Jiri Kosina
SUSE Labs
^ permalink raw reply related
* Re: [PATCH ghak90 (was ghak32) V4 01/10] audit: collect audit task parameters
From: Guenter Roeck @ 2019-01-04 22:04 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, ebiederm, luto, carlos, dhowells,
viro, simo, eparis, serge
In-Reply-To: <20190104145735.vlrw2fip5syn2exc@madcap2.tricolour.ca>
On Fri, Jan 04, 2019 at 09:57:35AM -0500, Richard Guy Briggs wrote:
> On 2019-01-03 18:50, Guenter Roeck wrote:
> > Hi Richard,
> >
> > On Tue, Jul 31, 2018 at 04:07:36PM -0400, Richard Guy Briggs wrote:
> > > The audit-related parameters in struct task_struct should ideally be
> > > collected together and accessed through a standard audit API.
> > >
> > > Collect the existing loginuid, sessionid and audit_context together in a
> > > new struct audit_task_info called "audit" in struct task_struct.
> > >
> > > Use kmem_cache to manage this pool of memory.
> > > Un-inline audit_free() to be able to always recover that memory.
> > >
> > > See: https://github.com/linux-audit/audit-kernel/issues/81
> > >
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> >
> > Overall I am not sure if keeping task_struct a bit smaller is worth
> > the added complexity, but I guess that is just me.
>
> The motivation was to consolidate all the audit bits into one pointer,
> isolating them from the rest of the kernel, restricting access only to
> helper functions to prevent abuse by other subsystems and trying to
> reduce kABI issues in the future. I agree it is a bit more complex. It
> was provoked by the need to add contid which seemed to make the most
> sense as a peer to loginuid and sessionid, and adding it to task_struct
> would have made it a bit too generic and available.
>
> This is addressed at some length by Paul Moore here in v2:
> https://lkml.org/lkml/2018/4/18/759
>
That makes sense. Thanks a lot for the clarification.
Guenter
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 01/10] audit: collect audit task parameters
From: Richard Guy Briggs @ 2019-01-04 14:57 UTC (permalink / raw)
To: Guenter Roeck
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, ebiederm, luto, carlos, dhowells,
viro, simo, eparis, serge
In-Reply-To: <20190104025006.GA15567@roeck-us.net>
On 2019-01-03 18:50, Guenter Roeck wrote:
> Hi Richard,
>
> On Tue, Jul 31, 2018 at 04:07:36PM -0400, Richard Guy Briggs wrote:
> > The audit-related parameters in struct task_struct should ideally be
> > collected together and accessed through a standard audit API.
> >
> > Collect the existing loginuid, sessionid and audit_context together in a
> > new struct audit_task_info called "audit" in struct task_struct.
> >
> > Use kmem_cache to manage this pool of memory.
> > Un-inline audit_free() to be able to always recover that memory.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/81
> >
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
>
> Overall I am not sure if keeping task_struct a bit smaller is worth
> the added complexity, but I guess that is just me.
The motivation was to consolidate all the audit bits into one pointer,
isolating them from the rest of the kernel, restricting access only to
helper functions to prevent abuse by other subsystems and trying to
reduce kABI issues in the future. I agree it is a bit more complex. It
was provoked by the need to add contid which seemed to make the most
sense as a peer to loginuid and sessionid, and adding it to task_struct
would have made it a bit too generic and available.
This is addressed at some length by Paul Moore here in v2:
https://lkml.org/lkml/2018/4/18/759
> Anyway, couple of nitpicks. Please feel free to ignore, and my apologies
> if some of all of the comments are duplicates.
Noted. They all look like reasonable improvements, particulaly the
unnecessary else and default return. Thanks. The double context check
may go away anyways based on the removal of audit_take_context() in
Paul's 2a1fe215e730 ("audit: use current whenever possible") which has
yet to be incorporated.
> Guenter
>
> > ---
> > include/linux/audit.h | 34 ++++++++++++++++++++++++----------
> > include/linux/sched.h | 5 +----
> > init/init_task.c | 3 +--
> > init/main.c | 2 ++
> > kernel/auditsc.c | 51 ++++++++++++++++++++++++++++++++++++++++++---------
> > kernel/fork.c | 4 +++-
> > 6 files changed, 73 insertions(+), 26 deletions(-)
> >
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 9334fbe..8964332 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -219,8 +219,15 @@ static inline void audit_log_task_info(struct audit_buffer *ab,
> >
> > /* These are defined in auditsc.c */
> > /* Public API */
>
> Not sure if the structure below belongs after "Public API".
> Is it part of the public API ?
>
> > +struct audit_task_info {
> > + kuid_t loginuid;
> > + unsigned int sessionid;
> > + struct audit_context *ctx;
> > +};
>
> Add empty line ?
>
> > +extern struct audit_task_info init_struct_audit;
> > +extern void __init audit_task_init(void);
> > extern int audit_alloc(struct task_struct *task);
> > -extern void __audit_free(struct task_struct *task);
> > +extern void audit_free(struct task_struct *task);
> > extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1,
> > unsigned long a2, unsigned long a3);
> > extern void __audit_syscall_exit(int ret_success, long ret_value);
> > @@ -242,12 +249,15 @@ extern void audit_seccomp_actions_logged(const char *names,
> >
> > static inline void audit_set_context(struct task_struct *task, struct audit_context *ctx)
> > {
> > - task->audit_context = ctx;
> > + task->audit->ctx = ctx;
> > }
> >
> > static inline struct audit_context *audit_context(void)
> > {
> > - return current->audit_context;
> > + if (current->audit)
> > + return current->audit->ctx;
> > + else
> > + return NULL;
>
> Unnecessary else (and static checkers may complain).
>
> > }
> >
> > static inline bool audit_dummy_context(void)
> > @@ -255,11 +265,7 @@ static inline bool audit_dummy_context(void)
> > void *p = audit_context();
> > return !p || *(int *)p;
> > }
> > -static inline void audit_free(struct task_struct *task)
> > -{
> > - if (unlikely(task->audit_context))
> > - __audit_free(task);
> > -}
> > +
> > static inline void audit_syscall_entry(int major, unsigned long a0,
> > unsigned long a1, unsigned long a2,
> > unsigned long a3)
> > @@ -331,12 +337,18 @@ extern int auditsc_get_stamp(struct audit_context *ctx,
> >
> > static inline kuid_t audit_get_loginuid(struct task_struct *tsk)
> > {
> > - return tsk->loginuid;
> > + if (tsk->audit)
> > + return tsk->audit->loginuid;
> > + else
> > + return INVALID_UID;
>
> Unnecessary else.
>
> > }
> >
> > static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
> > {
> > - return tsk->sessionid;
> > + if (tsk->audit)
> > + return tsk->audit->sessionid;
> > + else
> > + return AUDIT_SID_UNSET;
>
> Unnecessary else.
>
> > }
> >
> > extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
> > @@ -461,6 +473,8 @@ static inline void audit_fanotify(unsigned int response)
> > extern int audit_n_rules;
> > extern int audit_signals;
> > #else /* CONFIG_AUDITSYSCALL */
> > +static inline void __init audit_task_init(void)
> > +{ }
> > static inline int audit_alloc(struct task_struct *task)
> > {
> > return 0;
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index 87bf02d..e117272 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -30,7 +30,6 @@
> > #include <linux/rseq.h>
> >
> > /* task_struct member predeclarations (sorted alphabetically): */
> > -struct audit_context;
> > struct backing_dev_info;
> > struct bio_list;
> > struct blk_plug;
> > @@ -873,10 +872,8 @@ struct task_struct {
> >
> > struct callback_head *task_works;
> >
> > - struct audit_context *audit_context;
> > #ifdef CONFIG_AUDITSYSCALL
> > - kuid_t loginuid;
> > - unsigned int sessionid;
> > + struct audit_task_info *audit;
> > #endif
> > struct seccomp seccomp;
> >
> > diff --git a/init/init_task.c b/init/init_task.c
> > index 74f60ba..4058840 100644
> > --- a/init/init_task.c
> > +++ b/init/init_task.c
> > @@ -119,8 +119,7 @@ struct task_struct init_task
> > .thread_group = LIST_HEAD_INIT(init_task.thread_group),
> > .thread_node = LIST_HEAD_INIT(init_signals.thread_head),
> > #ifdef CONFIG_AUDITSYSCALL
> > - .loginuid = INVALID_UID,
> > - .sessionid = AUDIT_SID_UNSET,
> > + .audit = &init_struct_audit,
> > #endif
> > #ifdef CONFIG_PERF_EVENTS
> > .perf_event_mutex = __MUTEX_INITIALIZER(init_task.perf_event_mutex),
> > diff --git a/init/main.c b/init/main.c
> > index 3b4ada1..6aba171 100644
> > --- a/init/main.c
> > +++ b/init/main.c
> > @@ -92,6 +92,7 @@
> > #include <linux/rodata_test.h>
> > #include <linux/jump_label.h>
> > #include <linux/mem_encrypt.h>
> > +#include <linux/audit.h>
> >
> > #include <asm/io.h>
> > #include <asm/bugs.h>
> > @@ -721,6 +722,7 @@ asmlinkage __visible void __init start_kernel(void)
> > nsfs_init();
> > cpuset_init();
> > cgroup_init();
> > + audit_task_init();
> > taskstats_init_early();
> > delayacct_init();
> >
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index fb20746..88779a7 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -841,7 +841,7 @@ static inline struct audit_context *audit_take_context(struct task_struct *tsk,
> > int return_valid,
> > long return_code)
> > {
> > - struct audit_context *context = tsk->audit_context;
> > + struct audit_context *context = tsk->audit->ctx;
> >
> > if (!context)
> > return NULL;
> > @@ -926,6 +926,15 @@ static inline struct audit_context *audit_alloc_context(enum audit_state state)
> > return context;
> > }
> >
> > +static struct kmem_cache *audit_task_cache;
> > +
> > +void __init audit_task_init(void)
> > +{
> > + audit_task_cache = kmem_cache_create("audit_task",
> > + sizeof(struct audit_task_info),
> > + 0, SLAB_PANIC, NULL);
> > +}
> > +
> > /**
> > * audit_alloc - allocate an audit context block for a task
> > * @tsk: task
> > @@ -940,17 +949,28 @@ int audit_alloc(struct task_struct *tsk)
> > struct audit_context *context;
> > enum audit_state state;
> > char *key = NULL;
> > + struct audit_task_info *info;
> > +
> > + info = kmem_cache_zalloc(audit_task_cache, GFP_KERNEL);
> > + if (!info)
> > + return -ENOMEM;
> > + info->loginuid = audit_get_loginuid(current);
> > + info->sessionid = audit_get_sessionid(current);
> > + tsk->audit = info;
> >
> > if (likely(!audit_ever_enabled))
> > return 0; /* Return if not auditing. */
> >
> > state = audit_filter_task(tsk, &key);
> > if (state == AUDIT_DISABLED) {
> > + audit_set_context(tsk, NULL);
> > clear_tsk_thread_flag(tsk, TIF_SYSCALL_AUDIT);
> > return 0;
> > }
> >
> > if (!(context = audit_alloc_context(state))) {
> > + tsk->audit = NULL;
> > + kmem_cache_free(audit_task_cache, info);
> > kfree(key);
> > audit_log_lost("out of memory in audit_alloc");
> > return -ENOMEM;
> > @@ -962,6 +982,12 @@ int audit_alloc(struct task_struct *tsk)
> > return 0;
> > }
> >
> > +struct audit_task_info init_struct_audit = {
> > + .loginuid = INVALID_UID,
> > + .sessionid = AUDIT_SID_UNSET,
> > + .ctx = NULL,
> > +};
> > +
> > static inline void audit_free_context(struct audit_context *context)
> > {
> > audit_free_names(context);
> > @@ -1469,26 +1495,33 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts
> > }
> >
> > /**
> > - * __audit_free - free a per-task audit context
> > + * audit_free - free a per-task audit context
> > * @tsk: task whose audit context block to free
> > *
> > * Called from copy_process and do_exit
> > */
> > -void __audit_free(struct task_struct *tsk)
> > +void audit_free(struct task_struct *tsk)
> > {
> > struct audit_context *context;
> > + struct audit_task_info *info;
> >
> > context = audit_take_context(tsk, 0, 0);
> > - if (!context)
> > - return;
> > -
> > /* Check for system calls that do not go through the exit
> > * function (e.g., exit_group), then free context block.
> > * We use GFP_ATOMIC here because we might be doing this
> > * in the context of the idle thread */
> > /* that can happen only if we are called from do_exit() */
> > - if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT)
> > + if (context && context->in_syscall &&
> > + context->current_state == AUDIT_RECORD_CONTEXT)
> > audit_log_exit(context, tsk);
> > + /* Freeing the audit_task_info struct must be performed after
> > + * audit_log_exit() due to need for loginuid and sessionid.
> > + */
> > + info = tsk->audit;
> > + tsk->audit = NULL;
> > + kmem_cache_free(audit_task_cache, info);
> > + if (!context)
> > + return;
> > if (!list_empty(&context->killed_trees))
> > audit_kill_trees(&context->killed_trees);
>
> Looks kind of terrible with the repeated check if context is NULL.
> Maybe reorder ?
>
> context = audit_take_context(tsk, 0, 0);
> if (context) {
> /* do all the context work */
> }
> kmem_cache_free(audit_task_cache, tsk->audit);
> tsk->audit = NULL; // is that even necessary ?
>
> If "info" is really needed, ie if tsk (and tsk->audit) can be accessed
> from another thread in parallel, I'd be a bit concerned about the lack
> of sync() or similar after clearing tsk->audit.
>
> Another option might have been to separate audit_free() into
> audit_free_context() and audit_free_info().
>
> >
> > @@ -2071,8 +2104,8 @@ int audit_set_loginuid(kuid_t loginuid)
> > sessionid = (unsigned int)atomic_inc_return(&session_id);
> > }
> >
> > - task->sessionid = sessionid;
> > - task->loginuid = loginuid;
> > + task->audit->sessionid = sessionid;
> > + task->audit->loginuid = loginuid;
> > out:
> > audit_log_set_loginuid(oldloginuid, loginuid, oldsessionid, sessionid, rc);
> > return rc;
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index 9440d61..1bfb0ff 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -1721,7 +1721,9 @@ static __latent_entropy struct task_struct *copy_process(
> > p->start_time = ktime_get_ns();
> > p->real_start_time = ktime_get_boot_ns();
> > p->io_context = NULL;
> > - audit_set_context(p, NULL);
> > +#ifdef CONFIG_AUDITSYSCALL
> > + p->audit = NULL;
> > +#endif /* CONFIG_AUDITSYSCALL */
>
> audit_alloc() is called a bit later and sets p->audit, so I don't think
> this is really necessary.
>
> > cgroup_fork(p);
> > #ifdef CONFIG_NUMA
> > p->mempolicy = mpol_dup(p->mempolicy);
- 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
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 01/10] audit: collect audit task parameters
From: Guenter Roeck @ 2019-01-04 2:50 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, ebiederm, luto, carlos, dhowells,
viro, simo, eparis, serge
In-Reply-To: <8e617ab568df28a66dfbe3284452de186b42fb0f.1533065887.git.rgb@redhat.com>
Hi Richard,
On Tue, Jul 31, 2018 at 04:07:36PM -0400, Richard Guy Briggs wrote:
> The audit-related parameters in struct task_struct should ideally be
> collected together and accessed through a standard audit API.
>
> Collect the existing loginuid, sessionid and audit_context together in a
> new struct audit_task_info called "audit" in struct task_struct.
>
> Use kmem_cache to manage this pool of memory.
> Un-inline audit_free() to be able to always recover that memory.
>
> See: https://github.com/linux-audit/audit-kernel/issues/81
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
Overall I am not sure if keeping task_struct a bit smaller is worth
the added complexity, but I guess that is just me.
Anyway, couple of nitpicks. Please feel free to ignore, and my apologies
if some of all of the comments are duplicates.
Guenter
> ---
> include/linux/audit.h | 34 ++++++++++++++++++++++++----------
> include/linux/sched.h | 5 +----
> init/init_task.c | 3 +--
> init/main.c | 2 ++
> kernel/auditsc.c | 51 ++++++++++++++++++++++++++++++++++++++++++---------
> kernel/fork.c | 4 +++-
> 6 files changed, 73 insertions(+), 26 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 9334fbe..8964332 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -219,8 +219,15 @@ static inline void audit_log_task_info(struct audit_buffer *ab,
>
> /* These are defined in auditsc.c */
> /* Public API */
Not sure if the structure below belongs after "Public API".
Is it part of the public API ?
> +struct audit_task_info {
> + kuid_t loginuid;
> + unsigned int sessionid;
> + struct audit_context *ctx;
> +};
Add empty line ?
> +extern struct audit_task_info init_struct_audit;
> +extern void __init audit_task_init(void);
> extern int audit_alloc(struct task_struct *task);
> -extern void __audit_free(struct task_struct *task);
> +extern void audit_free(struct task_struct *task);
> extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1,
> unsigned long a2, unsigned long a3);
> extern void __audit_syscall_exit(int ret_success, long ret_value);
> @@ -242,12 +249,15 @@ extern void audit_seccomp_actions_logged(const char *names,
>
> static inline void audit_set_context(struct task_struct *task, struct audit_context *ctx)
> {
> - task->audit_context = ctx;
> + task->audit->ctx = ctx;
> }
>
> static inline struct audit_context *audit_context(void)
> {
> - return current->audit_context;
> + if (current->audit)
> + return current->audit->ctx;
> + else
> + return NULL;
Unnecessary else (and static checkers may complain).
> }
>
> static inline bool audit_dummy_context(void)
> @@ -255,11 +265,7 @@ static inline bool audit_dummy_context(void)
> void *p = audit_context();
> return !p || *(int *)p;
> }
> -static inline void audit_free(struct task_struct *task)
> -{
> - if (unlikely(task->audit_context))
> - __audit_free(task);
> -}
> +
> static inline void audit_syscall_entry(int major, unsigned long a0,
> unsigned long a1, unsigned long a2,
> unsigned long a3)
> @@ -331,12 +337,18 @@ extern int auditsc_get_stamp(struct audit_context *ctx,
>
> static inline kuid_t audit_get_loginuid(struct task_struct *tsk)
> {
> - return tsk->loginuid;
> + if (tsk->audit)
> + return tsk->audit->loginuid;
> + else
> + return INVALID_UID;
Unnecessary else.
> }
>
> static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
> {
> - return tsk->sessionid;
> + if (tsk->audit)
> + return tsk->audit->sessionid;
> + else
> + return AUDIT_SID_UNSET;
Unnecessary else.
> }
>
> extern void __audit_ipc_obj(struct kern_ipc_perm *ipcp);
> @@ -461,6 +473,8 @@ static inline void audit_fanotify(unsigned int response)
> extern int audit_n_rules;
> extern int audit_signals;
> #else /* CONFIG_AUDITSYSCALL */
> +static inline void __init audit_task_init(void)
> +{ }
> static inline int audit_alloc(struct task_struct *task)
> {
> return 0;
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 87bf02d..e117272 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -30,7 +30,6 @@
> #include <linux/rseq.h>
>
> /* task_struct member predeclarations (sorted alphabetically): */
> -struct audit_context;
> struct backing_dev_info;
> struct bio_list;
> struct blk_plug;
> @@ -873,10 +872,8 @@ struct task_struct {
>
> struct callback_head *task_works;
>
> - struct audit_context *audit_context;
> #ifdef CONFIG_AUDITSYSCALL
> - kuid_t loginuid;
> - unsigned int sessionid;
> + struct audit_task_info *audit;
> #endif
> struct seccomp seccomp;
>
> diff --git a/init/init_task.c b/init/init_task.c
> index 74f60ba..4058840 100644
> --- a/init/init_task.c
> +++ b/init/init_task.c
> @@ -119,8 +119,7 @@ struct task_struct init_task
> .thread_group = LIST_HEAD_INIT(init_task.thread_group),
> .thread_node = LIST_HEAD_INIT(init_signals.thread_head),
> #ifdef CONFIG_AUDITSYSCALL
> - .loginuid = INVALID_UID,
> - .sessionid = AUDIT_SID_UNSET,
> + .audit = &init_struct_audit,
> #endif
> #ifdef CONFIG_PERF_EVENTS
> .perf_event_mutex = __MUTEX_INITIALIZER(init_task.perf_event_mutex),
> diff --git a/init/main.c b/init/main.c
> index 3b4ada1..6aba171 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -92,6 +92,7 @@
> #include <linux/rodata_test.h>
> #include <linux/jump_label.h>
> #include <linux/mem_encrypt.h>
> +#include <linux/audit.h>
>
> #include <asm/io.h>
> #include <asm/bugs.h>
> @@ -721,6 +722,7 @@ asmlinkage __visible void __init start_kernel(void)
> nsfs_init();
> cpuset_init();
> cgroup_init();
> + audit_task_init();
> taskstats_init_early();
> delayacct_init();
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index fb20746..88779a7 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -841,7 +841,7 @@ static inline struct audit_context *audit_take_context(struct task_struct *tsk,
> int return_valid,
> long return_code)
> {
> - struct audit_context *context = tsk->audit_context;
> + struct audit_context *context = tsk->audit->ctx;
>
> if (!context)
> return NULL;
> @@ -926,6 +926,15 @@ static inline struct audit_context *audit_alloc_context(enum audit_state state)
> return context;
> }
>
> +static struct kmem_cache *audit_task_cache;
> +
> +void __init audit_task_init(void)
> +{
> + audit_task_cache = kmem_cache_create("audit_task",
> + sizeof(struct audit_task_info),
> + 0, SLAB_PANIC, NULL);
> +}
> +
> /**
> * audit_alloc - allocate an audit context block for a task
> * @tsk: task
> @@ -940,17 +949,28 @@ int audit_alloc(struct task_struct *tsk)
> struct audit_context *context;
> enum audit_state state;
> char *key = NULL;
> + struct audit_task_info *info;
> +
> + info = kmem_cache_zalloc(audit_task_cache, GFP_KERNEL);
> + if (!info)
> + return -ENOMEM;
> + info->loginuid = audit_get_loginuid(current);
> + info->sessionid = audit_get_sessionid(current);
> + tsk->audit = info;
>
> if (likely(!audit_ever_enabled))
> return 0; /* Return if not auditing. */
>
> state = audit_filter_task(tsk, &key);
> if (state == AUDIT_DISABLED) {
> + audit_set_context(tsk, NULL);
> clear_tsk_thread_flag(tsk, TIF_SYSCALL_AUDIT);
> return 0;
> }
>
> if (!(context = audit_alloc_context(state))) {
> + tsk->audit = NULL;
> + kmem_cache_free(audit_task_cache, info);
> kfree(key);
> audit_log_lost("out of memory in audit_alloc");
> return -ENOMEM;
> @@ -962,6 +982,12 @@ int audit_alloc(struct task_struct *tsk)
> return 0;
> }
>
> +struct audit_task_info init_struct_audit = {
> + .loginuid = INVALID_UID,
> + .sessionid = AUDIT_SID_UNSET,
> + .ctx = NULL,
> +};
> +
> static inline void audit_free_context(struct audit_context *context)
> {
> audit_free_names(context);
> @@ -1469,26 +1495,33 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts
> }
>
> /**
> - * __audit_free - free a per-task audit context
> + * audit_free - free a per-task audit context
> * @tsk: task whose audit context block to free
> *
> * Called from copy_process and do_exit
> */
> -void __audit_free(struct task_struct *tsk)
> +void audit_free(struct task_struct *tsk)
> {
> struct audit_context *context;
> + struct audit_task_info *info;
>
> context = audit_take_context(tsk, 0, 0);
> - if (!context)
> - return;
> -
> /* Check for system calls that do not go through the exit
> * function (e.g., exit_group), then free context block.
> * We use GFP_ATOMIC here because we might be doing this
> * in the context of the idle thread */
> /* that can happen only if we are called from do_exit() */
> - if (context->in_syscall && context->current_state == AUDIT_RECORD_CONTEXT)
> + if (context && context->in_syscall &&
> + context->current_state == AUDIT_RECORD_CONTEXT)
> audit_log_exit(context, tsk);
> + /* Freeing the audit_task_info struct must be performed after
> + * audit_log_exit() due to need for loginuid and sessionid.
> + */
> + info = tsk->audit;
> + tsk->audit = NULL;
> + kmem_cache_free(audit_task_cache, info);
> + if (!context)
> + return;
> if (!list_empty(&context->killed_trees))
> audit_kill_trees(&context->killed_trees);
Looks kind of terrible with the repeated check if context is NULL.
Maybe reorder ?
context = audit_take_context(tsk, 0, 0);
if (context) {
/* do all the context work */
}
kmem_cache_free(audit_task_cache, tsk->audit);
tsk->audit = NULL; // is that even necessary ?
If "info" is really needed, ie if tsk (and tsk->audit) can be accessed
from another thread in parallel, I'd be a bit concerned about the lack
of sync() or similar after clearing tsk->audit.
Another option might have been to separate audit_free() into
audit_free_context() and audit_free_info().
>
> @@ -2071,8 +2104,8 @@ int audit_set_loginuid(kuid_t loginuid)
> sessionid = (unsigned int)atomic_inc_return(&session_id);
> }
>
> - task->sessionid = sessionid;
> - task->loginuid = loginuid;
> + task->audit->sessionid = sessionid;
> + task->audit->loginuid = loginuid;
> out:
> audit_log_set_loginuid(oldloginuid, loginuid, oldsessionid, sessionid, rc);
> return rc;
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 9440d61..1bfb0ff 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1721,7 +1721,9 @@ static __latent_entropy struct task_struct *copy_process(
> p->start_time = ktime_get_ns();
> p->real_start_time = ktime_get_boot_ns();
> p->io_context = NULL;
> - audit_set_context(p, NULL);
> +#ifdef CONFIG_AUDITSYSCALL
> + p->audit = NULL;
> +#endif /* CONFIG_AUDITSYSCALL */
audit_alloc() is called a bit later and sets p->audit, so I don't think
this is really necessary.
> cgroup_fork(p);
> #ifdef CONFIG_NUMA
> p->mempolicy = mpol_dup(p->mempolicy);
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 01/10] audit: collect audit task parameters
From: Richard Guy Briggs @ 2019-01-03 20:38 UTC (permalink / raw)
To: Paul Moore
Cc: containers, linux-api, linux-audit, linux-kernel, ebiederm, luto,
dhowells, viro, simo, Eric Paris, Serge Hallyn
In-Reply-To: <CAHC9VhQTQuRnkF-8sJxwDvKbxJHvs5uLWsvLvL085SgyPiqLHQ@mail.gmail.com>
On 2019-01-03 15:33, Paul Moore wrote:
> On Thu, Jan 3, 2019 at 3:29 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > I'm not sure what's going on here, but it looks like HTML-encoded reply
> > quoting making the quoted text very difficult to read. All the previous
> > ">" have been converted to the HTML ">" encoding. Your most recent
> > reply text looks mostly fine.
>
> Not sure what happened either, I suspect gmail did something odd when
> I saved them as drafts, but it has never done that before. FWIW, I
> generally batch up individual review comments for complex patchsets as
> one often needs to review the entire set first before commenting.
>
> The most recent reply to patch 0/10 wasn't saved as a draft before sending.
Yeah, I noticed the last one was fine and wondered why it was different.
/me <3 mutt...
> > On 2019-01-03 15:10, Paul Moore wrote:
> > > On Thu, Nov 1, 2018 at 6:07 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > On 2018-10-19 19:15, Paul Moore wrote:
> > > > > On Sun, Aug 5, 2018 at 4:32 AM Richard Guy Briggs
> > > <rgb@redhat.com> wrote:
> > > > > > The audit-related parameters in struct task_struct
> > > should ideally be
> > > > > > collected together and accessed through a standard audit API.
> > > > > >
> > > > > > Collect the existing loginuid, sessionid and
> > > audit_context together in a
> > > > > > new struct audit_task_info called "audit" in struct task_struct.
> > > > > >
> > > > > > Use kmem_cache to manage this pool of memory.
> > > > > > Un-inline audit_free() to be able to always recover that memory.
> > > > > >
> > > > > > See: https://github.com/linux-audit/audit-kernel/issues/81
> > > > > >
> > > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > > > ---
> > > > > > include/linux/audit.h | 34 ++++++++++++++++++++++++----------
> > > > > > include/linux/sched.h | 5 +----
> > > > > > init/init_task.c | 3 +--
> > > > > > init/main.c | 2 ++
> > > > > > kernel/auditsc.c | 51
> > > ++++++++++++++++++++++++++++++++++++++++++---------
> > > > > > kernel/fork.c | 4 +++-
> > > > > > 6 files changed, 73 insertions(+), 26 deletions(-)
> > > > >
> > > > > ...
> > > > >
> > > > > > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > > > > > index 87bf02d..e117272 100644
> > > > > > --- a/include/linux/sched.h
> > > > > > +++ b/include/linux/sched.h
> > > > > > @@ -873,10 +872,8 @@ struct task_struct {
> > > > > >
> > > > > > struct callback_head *task_works;
> > > > > >
> > > > > > - struct audit_context *audit_context;
> > > > > > #ifdef CONFIG_AUDITSYSCALL
> > > > > > - kuid_t loginuid;
> > > > > > - unsigned int sessionid;
> > > > > > + struct audit_task_info *audit;
> > > > > > #endif
> > > > > > struct seccomp seccomp;
> > > > >
> > > > > Prior to this patch audit_context was available regardless of
> > > > > CONFIG_AUDITSYSCALL, after this patch the corresponding audit_context
> > > > > is only available when CONFIG_AUDITSYSCALL is defined.
> > > >
> > > > This was intentional since audit_context is not used when AUDITSYSCALL is
> > > > disabled. audit_alloc() was stubbed in that case to return 0.
> > > audit_context()
> > > > returned NULL.
> > > >
> > > > The fact that audit_context was still present in struct task_struct was an
> > > > oversight in the two patches already accepted:
> > > > ("audit: use inline function to get audit context")
> > > > ("audit: use inline function to get audit context")
> > > > that failed to hide or remove it from struct task_struct when it
> > > was no longer
> > > > relevant.
> > >
> > > Okay, in that case let's pull this out and fix this separately from
> > > the audit container ID patchset.
> > >
> > > > On further digging, loginuid and sessionid (and
> > > audit_log_session_info) should
> > > > be part of CONFIG_AUDIT scope and not CONFIG_AUDITSYSCALL since
> > > it is used in
> > > > CONFIG_CHANGE, ANOM_LINK, FEATURE_CHANGE(, INTEGRITY_RULE), none
> > > of which are
> > > > otherwise dependent on AUDITSYSCALL.
> > >
> > > This looks like something else we should fix independently from this patchset.
> > >
> > > > Looking ahead, contid should be treated like loginuid and
> > > sessionid, which are
> > > > currently only available when syscall auditting is.
> > >
> > > That seems reasonable. Eventually it would be great if we got rid of
> > > CONFIG_AUDITSYSCALL, but that is a separate issue, and something that
> > > is going to require work from the different arch/ABI folks to ensure
> > > everything is working properly.
> > >
> > > > Converting records from standalone to syscall and checking
> > > audit_dummy_context
> > > > changes the nature of CONFIG_AUDIT/!CONFIG_AUDITSYSCALL separation.
> > > > eg: ANOM_LINK accompanied by PATH record (which needed CWD addition to be
> > > > complete anyways)
> > > >
> > > > > > diff --git a/init/main.c b/init/main.c
> > > > > > index 3b4ada1..6aba171 100644
> > > > > > --- a/init/main.c
> > > > > > +++ b/init/main.c
> > > > > > @@ -92,6 +92,7 @@
> > > > > > #include <linux rodata_test.h="">
> > > > > > #include <linux jump_label.h="">
> > > > > > #include <linux mem_encrypt.h="">
> > > > > > +#include <linux audit.h="">
> > > > > >
> > > > > > #include <asm io.h="">
> > > > > > #include <asm bugs.h="">
> > > > > > @@ -721,6 +722,7 @@ asmlinkage __visible void __init
> > > start_kernel(void)
> > > > > > nsfs_init();
> > > > > > cpuset_init();
> > > > > > cgroup_init();
> > > > > > + audit_task_init();
> > > > > > taskstats_init_early();
> > > > > > delayacct_init();
> > > > >
> > > > > It seems like we would need either init_struct_audit or
> > > > > audit_task_init(), but not both, yes?
> > > >
> > > > One sets initial values of init task via an included struct,
> > > other makes a call
> > > > to create the kmem cache. Both seem appropriate to me unless we move the
> > > > initialization from a struct to assignments in audit_task_init(),
> > > but I'm not
> > > > that comfortable separating the audit init values from the rest of the
> > > > task_struct init task initializers (though there are other
> > > subsystems that need
> > > > to do so dynamically).
> > >
> > > My original thinking was focused on the use of init_struct_audit as an
> > > initializer when audit_task_init() was already creating a kmem_cache
> > > pool and a zero'd/init'd audit_task_info could be obtained via the
> > > usual kmem_cache functions. Alternatively, although I don't believe
> > > it would be recommended for this case, would be to use
> > > init_struct_audit as an init helper if we included the audit_task_info
> > > struct directly in the task_struct, as opposed to a pointer. What I
> > > missed was the simple fact that you're only using init_struct_audit
> > > for the init_task, which pretty much makes my original question rather
> > > silly :)
> > >
> > > --
> > > paul moore
> > > www.paul-moore.com
> >
> > - 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
> www.paul-moore.com
- 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
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 01/10] audit: collect audit task parameters
From: Paul Moore @ 2019-01-03 20:33 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, linux-audit, linux-kernel, ebiederm, luto,
dhowells, viro, simo, Eric Paris, Serge Hallyn
In-Reply-To: <20190103202938.ntdfqdmyrr7stzh2@madcap2.tricolour.ca>
On Thu, Jan 3, 2019 at 3:29 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> I'm not sure what's going on here, but it looks like HTML-encoded reply
> quoting making the quoted text very difficult to read. All the previous
> ">" have been converted to the HTML ">" encoding. Your most recent
> reply text looks mostly fine.
Not sure what happened either, I suspect gmail did something odd when
I saved them as drafts, but it has never done that before. FWIW, I
generally batch up individual review comments for complex patchsets as
one often needs to review the entire set first before commenting.
The most recent reply to patch 0/10 wasn't saved as a draft before sending.
> On 2019-01-03 15:10, Paul Moore wrote:
> > On Thu, Nov 1, 2018 at 6:07 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > On 2018-10-19 19:15, Paul Moore wrote:
> > > > On Sun, Aug 5, 2018 at 4:32 AM Richard Guy Briggs
> > <rgb@redhat.com> wrote:
> > > > > The audit-related parameters in struct task_struct
> > should ideally be
> > > > > collected together and accessed through a standard audit API.
> > > > >
> > > > > Collect the existing loginuid, sessionid and
> > audit_context together in a
> > > > > new struct audit_task_info called "audit" in struct task_struct.
> > > > >
> > > > > Use kmem_cache to manage this pool of memory.
> > > > > Un-inline audit_free() to be able to always recover that memory.
> > > > >
> > > > > See: https://github.com/linux-audit/audit-kernel/issues/81
> > > > >
> > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > > ---
> > > > > include/linux/audit.h | 34 ++++++++++++++++++++++++----------
> > > > > include/linux/sched.h | 5 +----
> > > > > init/init_task.c | 3 +--
> > > > > init/main.c | 2 ++
> > > > > kernel/auditsc.c | 51
> > ++++++++++++++++++++++++++++++++++++++++++---------
> > > > > kernel/fork.c | 4 +++-
> > > > > 6 files changed, 73 insertions(+), 26 deletions(-)
> > > >
> > > > ...
> > > >
> > > > > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > > > > index 87bf02d..e117272 100644
> > > > > --- a/include/linux/sched.h
> > > > > +++ b/include/linux/sched.h
> > > > > @@ -873,10 +872,8 @@ struct task_struct {
> > > > >
> > > > > struct callback_head *task_works;
> > > > >
> > > > > - struct audit_context *audit_context;
> > > > > #ifdef CONFIG_AUDITSYSCALL
> > > > > - kuid_t loginuid;
> > > > > - unsigned int sessionid;
> > > > > + struct audit_task_info *audit;
> > > > > #endif
> > > > > struct seccomp seccomp;
> > > >
> > > > Prior to this patch audit_context was available regardless of
> > > > CONFIG_AUDITSYSCALL, after this patch the corresponding audit_context
> > > > is only available when CONFIG_AUDITSYSCALL is defined.
> > >
> > > This was intentional since audit_context is not used when AUDITSYSCALL is
> > > disabled. audit_alloc() was stubbed in that case to return 0.
> > audit_context()
> > > returned NULL.
> > >
> > > The fact that audit_context was still present in struct task_struct was an
> > > oversight in the two patches already accepted:
> > > ("audit: use inline function to get audit context")
> > > ("audit: use inline function to get audit context")
> > > that failed to hide or remove it from struct task_struct when it
> > was no longer
> > > relevant.
> >
> > Okay, in that case let's pull this out and fix this separately from
> > the audit container ID patchset.
> >
> > > On further digging, loginuid and sessionid (and
> > audit_log_session_info) should
> > > be part of CONFIG_AUDIT scope and not CONFIG_AUDITSYSCALL since
> > it is used in
> > > CONFIG_CHANGE, ANOM_LINK, FEATURE_CHANGE(, INTEGRITY_RULE), none
> > of which are
> > > otherwise dependent on AUDITSYSCALL.
> >
> > This looks like something else we should fix independently from this patchset.
> >
> > > Looking ahead, contid should be treated like loginuid and
> > sessionid, which are
> > > currently only available when syscall auditting is.
> >
> > That seems reasonable. Eventually it would be great if we got rid of
> > CONFIG_AUDITSYSCALL, but that is a separate issue, and something that
> > is going to require work from the different arch/ABI folks to ensure
> > everything is working properly.
> >
> > > Converting records from standalone to syscall and checking
> > audit_dummy_context
> > > changes the nature of CONFIG_AUDIT/!CONFIG_AUDITSYSCALL separation.
> > > eg: ANOM_LINK accompanied by PATH record (which needed CWD addition to be
> > > complete anyways)
> > >
> > > > > diff --git a/init/main.c b/init/main.c
> > > > > index 3b4ada1..6aba171 100644
> > > > > --- a/init/main.c
> > > > > +++ b/init/main.c
> > > > > @@ -92,6 +92,7 @@
> > > > > #include <linux rodata_test.h="">
> > > > > #include <linux jump_label.h="">
> > > > > #include <linux mem_encrypt.h="">
> > > > > +#include <linux audit.h="">
> > > > >
> > > > > #include <asm io.h="">
> > > > > #include <asm bugs.h="">
> > > > > @@ -721,6 +722,7 @@ asmlinkage __visible void __init
> > start_kernel(void)
> > > > > nsfs_init();
> > > > > cpuset_init();
> > > > > cgroup_init();
> > > > > + audit_task_init();
> > > > > taskstats_init_early();
> > > > > delayacct_init();
> > > >
> > > > It seems like we would need either init_struct_audit or
> > > > audit_task_init(), but not both, yes?
> > >
> > > One sets initial values of init task via an included struct,
> > other makes a call
> > > to create the kmem cache. Both seem appropriate to me unless we move the
> > > initialization from a struct to assignments in audit_task_init(),
> > but I'm not
> > > that comfortable separating the audit init values from the rest of the
> > > task_struct init task initializers (though there are other
> > subsystems that need
> > > to do so dynamically).
> >
> > My original thinking was focused on the use of init_struct_audit as an
> > initializer when audit_task_init() was already creating a kmem_cache
> > pool and a zero'd/init'd audit_task_info could be obtained via the
> > usual kmem_cache functions. Alternatively, although I don't believe
> > it would be recommended for this case, would be to use
> > init_struct_audit as an init helper if we included the audit_task_info
> > struct directly in the task_struct, as opposed to a pointer. What I
> > missed was the simple fact that you're only using init_struct_audit
> > for the init_task, which pretty much makes my original question rather
> > silly :)
> >
> > --
> > paul moore
> > www.paul-moore.com
>
> - 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
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 01/10] audit: collect audit task parameters
From: Richard Guy Briggs @ 2019-01-03 20:29 UTC (permalink / raw)
To: Paul Moore
Cc: simo, linux-api, containers, linux-kernel, dhowells, linux-audit,
ebiederm, luto, Eric Paris, Serge Hallyn, viro
In-Reply-To: <CAHC9VhRU-Qss41CnV3mE32tXmZ3FQpy++t4yPUkh9CjuAzxYCA@mail.gmail.com>
I'm not sure what's going on here, but it looks like HTML-encoded reply
quoting making the quoted text very difficult to read. All the previous
">" have been converted to the HTML ">" encoding. Your most recent
reply text looks mostly fine.
On 2019-01-03 15:10, Paul Moore wrote:
> On Thu, Nov 1, 2018 at 6:07 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > On 2018-10-19 19:15, Paul Moore wrote:
> > > On Sun, Aug 5, 2018 at 4:32 AM Richard Guy Briggs
> <rgb@redhat.com> wrote:
> > > > The audit-related parameters in struct task_struct
> should ideally be
> > > > collected together and accessed through a standard audit API.
> > > >
> > > > Collect the existing loginuid, sessionid and
> audit_context together in a
> > > > new struct audit_task_info called "audit" in struct task_struct.
> > > >
> > > > Use kmem_cache to manage this pool of memory.
> > > > Un-inline audit_free() to be able to always recover that memory.
> > > >
> > > > See: https://github.com/linux-audit/audit-kernel/issues/81
> > > >
> > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > > ---
> > > > include/linux/audit.h | 34 ++++++++++++++++++++++++----------
> > > > include/linux/sched.h | 5 +----
> > > > init/init_task.c | 3 +--
> > > > init/main.c | 2 ++
> > > > kernel/auditsc.c | 51
> ++++++++++++++++++++++++++++++++++++++++++---------
> > > > kernel/fork.c | 4 +++-
> > > > 6 files changed, 73 insertions(+), 26 deletions(-)
> > >
> > > ...
> > >
> > > > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > > > index 87bf02d..e117272 100644
> > > > --- a/include/linux/sched.h
> > > > +++ b/include/linux/sched.h
> > > > @@ -873,10 +872,8 @@ struct task_struct {
> > > >
> > > > struct callback_head *task_works;
> > > >
> > > > - struct audit_context *audit_context;
> > > > #ifdef CONFIG_AUDITSYSCALL
> > > > - kuid_t loginuid;
> > > > - unsigned int sessionid;
> > > > + struct audit_task_info *audit;
> > > > #endif
> > > > struct seccomp seccomp;
> > >
> > > Prior to this patch audit_context was available regardless of
> > > CONFIG_AUDITSYSCALL, after this patch the corresponding audit_context
> > > is only available when CONFIG_AUDITSYSCALL is defined.
> >
> > This was intentional since audit_context is not used when AUDITSYSCALL is
> > disabled. audit_alloc() was stubbed in that case to return 0.
> audit_context()
> > returned NULL.
> >
> > The fact that audit_context was still present in struct task_struct was an
> > oversight in the two patches already accepted:
> > ("audit: use inline function to get audit context")
> > ("audit: use inline function to get audit context")
> > that failed to hide or remove it from struct task_struct when it
> was no longer
> > relevant.
>
> Okay, in that case let's pull this out and fix this separately from
> the audit container ID patchset.
>
> > On further digging, loginuid and sessionid (and
> audit_log_session_info) should
> > be part of CONFIG_AUDIT scope and not CONFIG_AUDITSYSCALL since
> it is used in
> > CONFIG_CHANGE, ANOM_LINK, FEATURE_CHANGE(, INTEGRITY_RULE), none
> of which are
> > otherwise dependent on AUDITSYSCALL.
>
> This looks like something else we should fix independently from this patchset.
>
> > Looking ahead, contid should be treated like loginuid and
> sessionid, which are
> > currently only available when syscall auditting is.
>
> That seems reasonable. Eventually it would be great if we got rid of
> CONFIG_AUDITSYSCALL, but that is a separate issue, and something that
> is going to require work from the different arch/ABI folks to ensure
> everything is working properly.
>
> > Converting records from standalone to syscall and checking
> audit_dummy_context
> > changes the nature of CONFIG_AUDIT/!CONFIG_AUDITSYSCALL separation.
> > eg: ANOM_LINK accompanied by PATH record (which needed CWD addition to be
> > complete anyways)
> >
> > > > diff --git a/init/main.c b/init/main.c
> > > > index 3b4ada1..6aba171 100644
> > > > --- a/init/main.c
> > > > +++ b/init/main.c
> > > > @@ -92,6 +92,7 @@
> > > > #include <linux rodata_test.h="">
> > > > #include <linux jump_label.h="">
> > > > #include <linux mem_encrypt.h="">
> > > > +#include <linux audit.h="">
> > > >
> > > > #include <asm io.h="">
> > > > #include <asm bugs.h="">
> > > > @@ -721,6 +722,7 @@ asmlinkage __visible void __init
> start_kernel(void)
> > > > nsfs_init();
> > > > cpuset_init();
> > > > cgroup_init();
> > > > + audit_task_init();
> > > > taskstats_init_early();
> > > > delayacct_init();
> > >
> > > It seems like we would need either init_struct_audit or
> > > audit_task_init(), but not both, yes?
> >
> > One sets initial values of init task via an included struct,
> other makes a call
> > to create the kmem cache. Both seem appropriate to me unless we move the
> > initialization from a struct to assignments in audit_task_init(),
> but I'm not
> > that comfortable separating the audit init values from the rest of the
> > task_struct init task initializers (though there are other
> subsystems that need
> > to do so dynamically).
>
> My original thinking was focused on the use of init_struct_audit as an
> initializer when audit_task_init() was already creating a kmem_cache
> pool and a zero'd/init'd audit_task_info could be obtained via the
> usual kmem_cache functions. Alternatively, although I don't believe
> it would be recommended for this case, would be to use
> init_struct_audit as an init helper if we included the audit_task_info
> struct directly in the task_struct, as opposed to a pointer. What I
> missed was the simple fact that you're only using init_struct_audit
> for the init_task, which pretty much makes my original question rather
> silly :)
>
> --
> paul moore
> www.paul-moore.com
- 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
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 00/10] audit: implement container identifier
From: Richard Guy Briggs @ 2019-01-03 20:20 UTC (permalink / raw)
To: Guenter Roeck
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, luto, carlos, viro, dhowells, simo,
eparis, serge, ebiederm
In-Reply-To: <20190103185849.GA25611@roeck-us.net>
On 2019-01-03 10:58, Guenter Roeck wrote:
> Hi Richard,
>
> On Thu, Jan 03, 2019 at 12:36:13PM -0500, Richard Guy Briggs wrote:
> > On 2019-01-03 08:15, Guenter Roeck wrote:
> > > Hi,
> > >
> > > On Tue, Jul 31, 2018 at 04:07:35PM -0400, Richard Guy Briggs wrote:
> > > > Implement kernel audit container identifier.
> > >
> > > I don't see a follow-up submission of this patch series. Has it been abandoned,
> > > or do I use the wrong search terms ?
> >
> > Guenter, thanks for your interest in this patchset. I haven't
> > abandoned it. I've pushed some updates to my own (ill-publicized)
> > public git repo. This effort has been going on more than 5 years with 8
>
> Oh man :-(. Not sure if I would be that patient.
Patience, subbornness, unjustified optimism, tenacity, inflexibility, who knows...
Are you talking about sticking with this particular problem, or delay
before checking in on a particular patch review?
> Can you point me to your repository ?
Sure. It hasn't been squashed and will be rebased.
git://toccata2.tricolour.ca/linux-2.6-rgb.git
I still have some write locks to check and work on.
> > previous revisions trying to document task namespaces and deciding that
> > was insufficient.
>
> My interest is mostly thanks to having some of the patches of your series
> in my incoming code review queue:
>
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1379654/3
Ok, interesting. Michael Halcrow had approached me in Vancouver at LSS
at the end of August and I regret not having had enough time to talk
with him further about it.
> As background, some of the patches in the series are needed by GCP (Google
> Cloud Platform) as a prerequisite for some security features. Having to
> maintain out-of-tree code is always a pain, even more so in a subsystem
> related to security. So it would be quite useful to understand if we are
> going to be stuck with this forever or if there is a change for the code
> to find its way upstream. Also, it would be useful to know if there are
> some upcoming changes/improvements which should be included in our version.
There are likely more changes coming, but I don't expect them to be
that drastic a departure from the original design. There were some
changes in the implementation based on unforseen issues raised once
coding started (which is part of the process). Upstream patch review
would be the most helpful in keeping this stuff moving.
David Howells also had some interesting ideas and patches to try to
address some of these problems and he's still working on a prerequisite
patchset to get it upstream before returning to his container identifier
patchset. It is moving slowly.
> Thanks,
> Guenter
>
> > For this patchset I waited 11.5 weeks (80 days, Jules Verne anyone?)
> > before the primary intended maintainer did the first review, then I
> > responded within 2 weeks with further questions and a followup patch
> > proposal and then waited another 8 weeks for any response before adding
> > another query for that followup patch proposal review at which point I
> > got a rude answer saying I had disappointed and exhausted the
> > maintainer's goodwill with some hints at how to proceed just before new
> > year's.
> >
> > I'd be delighted with other upstream review to get other angles and to
> > take some of the load and responsibility off the primary maintainer.
> >
> > I expect to submit a v5 within a week without having had those questions
> > directly answered, but with some ideas of what to check and verify
> > before I resubmit. Most of the changes have been sitting in that branch
> > for two months, already rebased one kernel version and will need
> > updating again.
> >
> > > Thanks,
> > > Guenter
> > >
> > > > This patchset is a fourth based on the proposal document (V3)
> > > > posted:
> > > > https://www.redhat.com/archives/linux-audit/2018-January/msg00014.html
> > > >
> > > > The first patch is the last patch from ghak81 that is included here as a
> > > > convenience.
> > > >
> > > > The second patch implements the proc fs write to set the audit container
> > > > identifier of a process, emitting an AUDIT_CONTAINER_OP record to announce the
> > > > registration of that audit container identifier on that process. This patch
> > > > requires userspace support for record acceptance and proper type
> > > > display.
> > > >
> > > > The third implements the auxiliary record AUDIT_CONTAINER if an
> > > > audit container identifier is identifiable with an event. This patch
> > > > requires userspace support for proper type display.
> > > >
> > > > The 4th adds signal and ptrace support.
> > > >
> > > > The 5th creates a local audit context to be able to bind a standalone
> > > > record with a locally created auxiliary record.
> > > >
> > > > The 6th patch adds audit container identifier records to the tty
> > > > standalone record.
> > > >
> > > > The 7th adds audit container identifier filtering to the exit,
> > > > exclude and user lists. This patch adds the AUDIT_CONTID field and
> > > > requires auditctl userspace support for the --contid option.
> > > >
> > > > The 8th adds network namespace audit container identifier labelling
> > > > based on member tasks' audit container identifier labels.
> > > >
> > > > The 9th adds audit container identifier support to standalone netfilter
> > > > records that don't have a task context and lists each container to which
> > > > that net namespace belongs.
> > > >
> > > > The 10th implements reading the audit container identifier from the proc
> > > > filesystem for debugging. This patch isn't planned for upstream
> > > > inclusion.
> > > >
> > > >
> > > > Example: Set an audit container identifier of 123456 to the "sleep" task:
> > > >
> > > > sleep 2&
> > > > child=$!
> > > > echo 123456 > /proc/$child/audit_containerid; echo $?
> > > > ausearch -ts recent -m container
> > > > echo child:$child contid:$( cat /proc/$child/audit_containerid)
> > > >
> > > > This should produce a record such as:
> > > >
> > > > type=CONTAINER_OP msg=audit(2018-06-06 12:39:29.636:26949) : op=set opid=2209 old-contid=18446744073709551615 contid=123456 pid=628 auid=root uid=root tty=ttyS0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 comm=bash exe=/usr/bin/bash res=yes
> > > >
> > > >
> > > > Example: Set a filter on an audit container identifier 123459 on /tmp/tmpcontainerid:
> > > >
> > > > contid=123459
> > > > key=tmpcontainerid
> > > > auditctl -a exit,always -F dir=/tmp -F perm=wa -F contid=$contid -F key=$key
> > > > perl -e "sleep 1; open(my \$tmpfile, '>', \"/tmp/$key\"); close(\$tmpfile);" &
> > > > child=$!
> > > > echo $contid > /proc/$child/audit_containerid
> > > > sleep 2
> > > > ausearch -i -ts recent -k $key
> > > > auditctl -d exit,always -F dir=/tmp -F perm=wa -F contid=$contid -F key=$key
> > > > rm -f /tmp/$key
> > > >
> > > > This should produce an event such as:
> > > >
> > > > type=CONTAINER msg=audit(2018-06-06 12:46:31.707:26953) : op=task contid=123459
> > > > type=PROCTITLE msg=audit(2018-06-06 12:46:31.707:26953) : proctitle=perl -e sleep 1; open(my $tmpfile, '>', "/tmp/tmpcontainerid"); close($tmpfile);
> > > > type=PATH msg=audit(2018-06-06 12:46:31.707:26953) : item=1 name=/tmp/tmpcontainerid inode=25656 dev=00:26 mode=file,644 ouid=root ogid=root rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=CREATE cap_fp=none cap_fi=none cap_fe=0 cap_fver=0
> > > > type=PATH msg=audit(2018-06-06 12:46:31.707:26953) : item=0 name=/tmp/ inode=8985 dev=00:26 mode=dir,sticky,777 ouid=root ogid=root rdev=00:00 obj=system_u:object_r:tmp_t:s0 nametype=PARENT cap_fp=none cap_fi=none cap_fe=0 cap_fver=0
> > > > type=CWD msg=audit(2018-06-06 12:46:31.707:26953) : cwd=/root
> > > > type=SYSCALL msg=audit(2018-06-06 12:46:31.707:26953) : arch=x86_64 syscall=openat success=yes exit=3 a0=0xffffffffffffff9c a1=0x5621f2b81900 a2=O_WRONLY|O_CREAT|O_TRUNC a3=0x1b6 items=2 ppid=628 pid=2232 auid=root uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=ttyS0 ses=1 comm=perl exe=/usr/bin/perl subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=tmpcontainerid
> > > >
> > > >
> > > > Includes: https://github.com/linux-audit/audit-kernel/issues/81
> > > > See: https://github.com/linux-audit/audit-kernel/issues/90
> > > > See: https://github.com/linux-audit/audit-userspace/issues/40
> > > > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > > > See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> > > >
> > > > Changelog:
> > > >
> > > > v4
> > > > - preface set with ghak81:"collect audit task parameters"
> > > > - add shallyn and sgrubb acks
> > > > - rename feature bitmap macro
> > > > - rename cid_valid() to audit_contid_valid()
> > > > - rename AUDIT_CONTAINER_ID to AUDIT_CONTAINER_OP
> > > > - delete audit_get_contid_list() from headers
> > > > - move work into inner if, delete "found"
> > > > - change netns contid list function names
> > > > - move exports for audit_log_contid audit_alloc_local audit_free_context to non-syscall patch
> > > > - list contids CSV
> > > > - pass in gfp flags to audit_alloc_local() (fix audit_alloc_context callers)
> > > > - use "local" in lieu of abusing in_syscall for auditsc_get_stamp()
> > > > - read_lock(&tasklist_lock) around children and thread check
> > > > - task_lock(tsk) should be taken before first check of tsk->audit
> > > > - add spin lock to contid list in aunet
> > > > - restrict /proc read to CAP_AUDIT_CONTROL
> > > > - remove set again prohibition and inherited flag
> > > > - delete contidion spelling fix from patchset, send to netdev/linux-wireless
> > > >
> > > > v3
> > > > - switched from containerid in task_struct to audit_task_info (depends on ghak81)
> > > > - drop INVALID_CID in favour of only AUDIT_CID_UNSET
> > > > - check for !audit_task_info, throw -ENOPROTOOPT on set
> > > > - changed -EPERM to -EEXIST for parent check
> > > > - return AUDIT_CID_UNSET if !audit_enabled
> > > > - squash child/thread check patch into AUDIT_CONTAINER_ID patch
> > > > - changed -EPERM to -EBUSY for child check
> > > > - separate child and thread checks, use -EALREADY for latter
> > > > - move addition of op= from ptrace/signal patch to AUDIT_CONTAINER patch
> > > > - fix && to || bashism in ptrace/signal patch
> > > > - uninline and export function for audit_free_context()
> > > > - drop CONFIG_CHANGE, FEATURE_CHANGE, ANOM_ABEND, ANOM_SECCOMP patches
> > > > - move audit_enabled check (xt_AUDIT)
> > > > - switched from containerid list in struct net to net_generic's struct audit_net
> > > > - move containerid list iteration into audit (xt_AUDIT)
> > > > - create function to move namespace switch into audit
> > > > - switched /proc/PID/ entry from containerid to audit_containerid
> > > > - call kzalloc with GFP_ATOMIC on in_atomic() in audit_alloc_context()
> > > > - call kzalloc with GFP_ATOMIC on in_atomic() in audit_log_container_info()
> > > > - use xt_net(par) instead of sock_net(skb->sk) to get net
> > > > - switched record and field names: initial CONTAINER_ID, aux CONTAINER, field CONTID
> > > > - allow to set own contid
> > > > - open code audit_set_containerid
> > > > - add contid inherited flag
> > > > - ccontainerid and pcontainerid eliminated due to inherited flag
> > > > - change name of container list funcitons
> > > > - rename containerid to contid
> > > > - convert initial container record to syscall aux
> > > > - fix spelling mistake of contidion in net/rfkill/core.c to avoid contid name collision
> > > >
> > > > v2
> > > > - add check for children and threads
> > > > - add network namespace container identifier list
> > > > - add NETFILTER_PKT audit container identifier logging
> > > > - patch description and documentation clean-up and example
> > > > - reap unused ppid
> > > >
> > > > Richard Guy Briggs (10):
> > > > audit: collect audit task parameters
> > > > audit: add container id
> > > > audit: log container info of syscalls
> > > > audit: add containerid support for ptrace and signals
> > > > audit: add support for non-syscall auxiliary records
> > > > audit: add containerid support for tty_audit
> > > > audit: add containerid filtering
> > > > audit: add support for containerid to network namespaces
> > > > audit: NETFILTER_PKT: record each container ID associated with a netNS
> > > > debug audit: read container ID of a process
> > > >
> > > > drivers/tty/tty_audit.c | 5 +-
> > > > fs/proc/base.c | 56 ++++++++++++++
> > > > include/linux/audit.h | 95 ++++++++++++++++++++---
> > > > include/linux/sched.h | 5 +-
> > > > include/uapi/linux/audit.h | 8 +-
> > > > init/init_task.c | 3 +-
> > > > init/main.c | 2 +
> > > > kernel/audit.c | 137 +++++++++++++++++++++++++++++++++
> > > > kernel/audit.h | 4 +
> > > > kernel/auditfilter.c | 47 ++++++++++++
> > > > kernel/auditsc.c | 183 ++++++++++++++++++++++++++++++++++++++++-----
> > > > kernel/fork.c | 4 +-
> > > > kernel/nsproxy.c | 4 +
> > > > net/netfilter/xt_AUDIT.c | 12 ++-
> > > > 14 files changed, 526 insertions(+), 39 deletions(-)
> > > >
> > > > --
> > > > 1.8.3.1
> > > >
> >
> > - 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
- 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
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 00/10] audit: implement container identifier
From: Paul Moore @ 2019-01-03 20:12 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Guenter Roeck, ebiederm, simo, carlos, linux-api, containers,
LKML, dhowells, Linux-Audit Mailing List, netfilter-devel, viro,
luto, netdev, linux-fsdevel, Eric Paris, Serge Hallyn
In-Reply-To: <20190103173613.jb5kukysslozytwg@madcap2.tricolour.ca>
On Thu, Jan 3, 2019 at 12:36 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2019-01-03 08:15, Guenter Roeck wrote:
> > Hi,
> >
> > On Tue, Jul 31, 2018 at 04:07:35PM -0400, Richard Guy Briggs wrote:
> > > Implement kernel audit container identifier.
> >
> > I don't see a follow-up submission of this patch series. Has it been abandoned,
> > or do I use the wrong search terms ?
>
> Guenter, thanks for your interest in this patchset. I haven't
> abandoned it. I've pushed some updates to my own (ill-publicized)
> public git repo. This effort has been going on more than 5 years with 8
> previous revisions trying to document task namespaces and deciding that
> was insufficient.
>
> For this patchset I waited 11.5 weeks (80 days, Jules Verne anyone?)
> before the primary intended maintainer did the first review, then I
> responded within 2 weeks with further questions and a followup patch
> proposal and then waited another 8 weeks for any response before adding
> another query for that followup patch proposal review at which point I
> got a rude answer saying I had disappointed and exhausted the
> maintainer's goodwill with some hints at how to proceed just before new
> year's.
For what it is worth, I've found your emails to me to be rather "rude"
as well (to borrow the term), and I responded with what I felt was
appropriate. Perhaps our interactions may have been seen as overly,
or quickly, harsh but I would remind those that we have several years
of history that extends far beyond the lists which obviously affects
how we interact. Our expectations for each other are clearly higher
than either of us are delivering, so I'm going to suggest what I've
suggested before, albeit privately: let's stick to the code, that's
where we can find common ground.
There were only a few outstanding threads/questions from your last
posting, you should have responses to those sitting in your inbox now.
> I'd be delighted with other upstream review to get other angles and to
> take some of the load and responsibility off the primary maintainer.
>
> I expect to submit a v5 within a week without having had those questions
> directly answered, but with some ideas of what to check and verify
> before I resubmit. Most of the changes have been sitting in that branch
> for two months, already rebased one kernel version and will need
> updating again.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 01/10] audit: collect audit task parameters
From: Paul Moore @ 2019-01-03 20:10 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, linux-audit, linux-kernel, ebiederm, luto,
dhowells, viro, simo, Eric Paris, Serge Hallyn
In-Reply-To: <20181101220724.ggkqyf5kjv7lhabx@madcap2.tricolour.ca>
On Thu, Nov 1, 2018 at 6:07 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2018-10-19 19:15, Paul Moore wrote:
> > On Sun, Aug 5, 2018 at 4:32 AM Richard Guy Briggs
<rgb@redhat.com> wrote:
> > > The audit-related parameters in struct task_struct
should ideally be
> > > collected together and accessed through a standard audit API.
> > >
> > > Collect the existing loginuid, sessionid and
audit_context together in a
> > > new struct audit_task_info called "audit" in struct task_struct.
> > >
> > > Use kmem_cache to manage this pool of memory.
> > > Un-inline audit_free() to be able to always recover that memory.
> > >
> > > See: https://github.com/linux-audit/audit-kernel/issues/81
> > >
> > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > > ---
> > > include/linux/audit.h | 34 ++++++++++++++++++++++++----------
> > > include/linux/sched.h | 5 +----
> > > init/init_task.c | 3 +--
> > > init/main.c | 2 ++
> > > kernel/auditsc.c | 51
++++++++++++++++++++++++++++++++++++++++++---------
> > > kernel/fork.c | 4 +++-
> > > 6 files changed, 73 insertions(+), 26 deletions(-)
> >
> > ...
> >
> > > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > > index 87bf02d..e117272 100644
> > > --- a/include/linux/sched.h
> > > +++ b/include/linux/sched.h
> > > @@ -873,10 +872,8 @@ struct task_struct {
> > >
> > > struct callback_head *task_works;
> > >
> > > - struct audit_context *audit_context;
> > > #ifdef CONFIG_AUDITSYSCALL
> > > - kuid_t loginuid;
> > > - unsigned int sessionid;
> > > + struct audit_task_info *audit;
> > > #endif
> > > struct seccomp seccomp;
> >
> > Prior to this patch audit_context was available regardless of
> > CONFIG_AUDITSYSCALL, after this patch the corresponding audit_context
> > is only available when CONFIG_AUDITSYSCALL is defined.
>
> This was intentional since audit_context is not used when AUDITSYSCALL is
> disabled. audit_alloc() was stubbed in that case to return 0.
audit_context()
> returned NULL.
>
> The fact that audit_context was still present in struct task_struct was an
> oversight in the two patches already accepted:
> ("audit: use inline function to get audit context")
> ("audit: use inline function to get audit context")
> that failed to hide or remove it from struct task_struct when it
was no longer
> relevant.
Okay, in that case let's pull this out and fix this separately from
the audit container ID patchset.
> On further digging, loginuid and sessionid (and
audit_log_session_info) should
> be part of CONFIG_AUDIT scope and not CONFIG_AUDITSYSCALL since
it is used in
> CONFIG_CHANGE, ANOM_LINK, FEATURE_CHANGE(, INTEGRITY_RULE), none
of which are
> otherwise dependent on AUDITSYSCALL.
This looks like something else we should fix independently from this patchset.
> Looking ahead, contid should be treated like loginuid and
sessionid, which are
> currently only available when syscall auditting is.
That seems reasonable. Eventually it would be great if we got rid of
CONFIG_AUDITSYSCALL, but that is a separate issue, and something that
is going to require work from the different arch/ABI folks to ensure
everything is working properly.
> Converting records from standalone to syscall and checking
audit_dummy_context
> changes the nature of CONFIG_AUDIT/!CONFIG_AUDITSYSCALL separation.
> eg: ANOM_LINK accompanied by PATH record (which needed CWD addition to be
> complete anyways)
>
> > > diff --git a/init/main.c b/init/main.c
> > > index 3b4ada1..6aba171 100644
> > > --- a/init/main.c
> > > +++ b/init/main.c
> > > @@ -92,6 +92,7 @@
> > > #include <linux rodata_test.h="">
> > > #include <linux jump_label.h="">
> > > #include <linux mem_encrypt.h="">
> > > +#include <linux audit.h="">
> > >
> > > #include <asm io.h="">
> > > #include <asm bugs.h="">
> > > @@ -721,6 +722,7 @@ asmlinkage __visible void __init
start_kernel(void)
> > > nsfs_init();
> > > cpuset_init();
> > > cgroup_init();
> > > + audit_task_init();
> > > taskstats_init_early();
> > > delayacct_init();
> >
> > It seems like we would need either init_struct_audit or
> > audit_task_init(), but not both, yes?
>
> One sets initial values of init task via an included struct,
other makes a call
> to create the kmem cache. Both seem appropriate to me unless we move the
> initialization from a struct to assignments in audit_task_init(),
but I'm not
> that comfortable separating the audit init values from the rest of the
> task_struct init task initializers (though there are other
subsystems that need
> to do so dynamically).
My original thinking was focused on the use of init_struct_audit as an
initializer when audit_task_init() was already creating a kmem_cache
pool and a zero'd/init'd audit_task_info could be obtained via the
usual kmem_cache functions. Alternatively, although I don't believe
it would be recommended for this case, would be to use
init_struct_audit as an init helper if we included the audit_task_info
struct directly in the task_struct, as opposed to a pointer. What I
missed was the simple fact that you're only using init_struct_audit
for the init_task, which pretty much makes my original question rather
silly :)
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH ghak90 (was ghak32) V4 00/10] audit: implement container identifier
From: Guenter Roeck @ 2019-01-03 18:58 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, luto, carlos, viro, dhowells, simo,
eparis, serge, ebiederm
In-Reply-To: <20190103173613.jb5kukysslozytwg@madcap2.tricolour.ca>
Hi Richard,
On Thu, Jan 03, 2019 at 12:36:13PM -0500, Richard Guy Briggs wrote:
> On 2019-01-03 08:15, Guenter Roeck wrote:
> > Hi,
> >
> > On Tue, Jul 31, 2018 at 04:07:35PM -0400, Richard Guy Briggs wrote:
> > > Implement kernel audit container identifier.
> >
> > I don't see a follow-up submission of this patch series. Has it been abandoned,
> > or do I use the wrong search terms ?
>
> Guenter, thanks for your interest in this patchset. I haven't
> abandoned it. I've pushed some updates to my own (ill-publicized)
> public git repo. This effort has been going on more than 5 years with 8
Oh man :-(. Not sure if I would be that patient.
Can you point me to your repository ?
> previous revisions trying to document task namespaces and deciding that
> was insufficient.
>
My interest is mostly thanks to having some of the patches of your series
in my incoming code review queue:
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/1379654/3
As background, some of the patches in the series are needed by GCP (Google
Cloud Platform) as a prerequisite for some security features. Having to
maintain out-of-tree code is always a pain, even more so in a subsystem
related to security. So it would be quite useful to understand if we are
going to be stuck with this forever or if there is a change for the code
to find its way upstream. Also, it would be useful to know if there are
some upcoming changes/improvements which should be included in our version.
Thanks,
Guenter
> For this patchset I waited 11.5 weeks (80 days, Jules Verne anyone?)
> before the primary intended maintainer did the first review, then I
> responded within 2 weeks with further questions and a followup patch
> proposal and then waited another 8 weeks for any response before adding
> another query for that followup patch proposal review at which point I
> got a rude answer saying I had disappointed and exhausted the
> maintainer's goodwill with some hints at how to proceed just before new
> year's.
>
> I'd be delighted with other upstream review to get other angles and to
> take some of the load and responsibility off the primary maintainer.
>
> I expect to submit a v5 within a week without having had those questions
> directly answered, but with some ideas of what to check and verify
> before I resubmit. Most of the changes have been sitting in that branch
> for two months, already rebased one kernel version and will need
> updating again.
>
> > Thanks,
> > Guenter
> >
> > > This patchset is a fourth based on the proposal document (V3)
> > > posted:
> > > https://www.redhat.com/archives/linux-audit/2018-January/msg00014.html
> > >
> > > The first patch is the last patch from ghak81 that is included here as a
> > > convenience.
> > >
> > > The second patch implements the proc fs write to set the audit container
> > > identifier of a process, emitting an AUDIT_CONTAINER_OP record to announce the
> > > registration of that audit container identifier on that process. This patch
> > > requires userspace support for record acceptance and proper type
> > > display.
> > >
> > > The third implements the auxiliary record AUDIT_CONTAINER if an
> > > audit container identifier is identifiable with an event. This patch
> > > requires userspace support for proper type display.
> > >
> > > The 4th adds signal and ptrace support.
> > >
> > > The 5th creates a local audit context to be able to bind a standalone
> > > record with a locally created auxiliary record.
> > >
> > > The 6th patch adds audit container identifier records to the tty
> > > standalone record.
> > >
> > > The 7th adds audit container identifier filtering to the exit,
> > > exclude and user lists. This patch adds the AUDIT_CONTID field and
> > > requires auditctl userspace support for the --contid option.
> > >
> > > The 8th adds network namespace audit container identifier labelling
> > > based on member tasks' audit container identifier labels.
> > >
> > > The 9th adds audit container identifier support to standalone netfilter
> > > records that don't have a task context and lists each container to which
> > > that net namespace belongs.
> > >
> > > The 10th implements reading the audit container identifier from the proc
> > > filesystem for debugging. This patch isn't planned for upstream
> > > inclusion.
> > >
> > >
> > > Example: Set an audit container identifier of 123456 to the "sleep" task:
> > >
> > > sleep 2&
> > > child=$!
> > > echo 123456 > /proc/$child/audit_containerid; echo $?
> > > ausearch -ts recent -m container
> > > echo child:$child contid:$( cat /proc/$child/audit_containerid)
> > >
> > > This should produce a record such as:
> > >
> > > type=CONTAINER_OP msg=audit(2018-06-06 12:39:29.636:26949) : op=set opid=2209 old-contid=18446744073709551615 contid=123456 pid=628 auid=root uid=root tty=ttyS0 ses=1 subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 comm=bash exe=/usr/bin/bash res=yes
> > >
> > >
> > > Example: Set a filter on an audit container identifier 123459 on /tmp/tmpcontainerid:
> > >
> > > contid=123459
> > > key=tmpcontainerid
> > > auditctl -a exit,always -F dir=/tmp -F perm=wa -F contid=$contid -F key=$key
> > > perl -e "sleep 1; open(my \$tmpfile, '>', \"/tmp/$key\"); close(\$tmpfile);" &
> > > child=$!
> > > echo $contid > /proc/$child/audit_containerid
> > > sleep 2
> > > ausearch -i -ts recent -k $key
> > > auditctl -d exit,always -F dir=/tmp -F perm=wa -F contid=$contid -F key=$key
> > > rm -f /tmp/$key
> > >
> > > This should produce an event such as:
> > >
> > > type=CONTAINER msg=audit(2018-06-06 12:46:31.707:26953) : op=task contid=123459
> > > type=PROCTITLE msg=audit(2018-06-06 12:46:31.707:26953) : proctitle=perl -e sleep 1; open(my $tmpfile, '>', "/tmp/tmpcontainerid"); close($tmpfile);
> > > type=PATH msg=audit(2018-06-06 12:46:31.707:26953) : item=1 name=/tmp/tmpcontainerid inode=25656 dev=00:26 mode=file,644 ouid=root ogid=root rdev=00:00 obj=unconfined_u:object_r:user_tmp_t:s0 nametype=CREATE cap_fp=none cap_fi=none cap_fe=0 cap_fver=0
> > > type=PATH msg=audit(2018-06-06 12:46:31.707:26953) : item=0 name=/tmp/ inode=8985 dev=00:26 mode=dir,sticky,777 ouid=root ogid=root rdev=00:00 obj=system_u:object_r:tmp_t:s0 nametype=PARENT cap_fp=none cap_fi=none cap_fe=0 cap_fver=0
> > > type=CWD msg=audit(2018-06-06 12:46:31.707:26953) : cwd=/root
> > > type=SYSCALL msg=audit(2018-06-06 12:46:31.707:26953) : arch=x86_64 syscall=openat success=yes exit=3 a0=0xffffffffffffff9c a1=0x5621f2b81900 a2=O_WRONLY|O_CREAT|O_TRUNC a3=0x1b6 items=2 ppid=628 pid=2232 auid=root uid=root gid=root euid=root suid=root fsuid=root egid=root sgid=root fsgid=root tty=ttyS0 ses=1 comm=perl exe=/usr/bin/perl subj=unconfined_u:unconfined_r:unconfined_t:s0-s0:c0.c1023 key=tmpcontainerid
> > >
> > >
> > > Includes: https://github.com/linux-audit/audit-kernel/issues/81
> > > See: https://github.com/linux-audit/audit-kernel/issues/90
> > > See: https://github.com/linux-audit/audit-userspace/issues/40
> > > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > > See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> > >
> > > Changelog:
> > >
> > > v4
> > > - preface set with ghak81:"collect audit task parameters"
> > > - add shallyn and sgrubb acks
> > > - rename feature bitmap macro
> > > - rename cid_valid() to audit_contid_valid()
> > > - rename AUDIT_CONTAINER_ID to AUDIT_CONTAINER_OP
> > > - delete audit_get_contid_list() from headers
> > > - move work into inner if, delete "found"
> > > - change netns contid list function names
> > > - move exports for audit_log_contid audit_alloc_local audit_free_context to non-syscall patch
> > > - list contids CSV
> > > - pass in gfp flags to audit_alloc_local() (fix audit_alloc_context callers)
> > > - use "local" in lieu of abusing in_syscall for auditsc_get_stamp()
> > > - read_lock(&tasklist_lock) around children and thread check
> > > - task_lock(tsk) should be taken before first check of tsk->audit
> > > - add spin lock to contid list in aunet
> > > - restrict /proc read to CAP_AUDIT_CONTROL
> > > - remove set again prohibition and inherited flag
> > > - delete contidion spelling fix from patchset, send to netdev/linux-wireless
> > >
> > > v3
> > > - switched from containerid in task_struct to audit_task_info (depends on ghak81)
> > > - drop INVALID_CID in favour of only AUDIT_CID_UNSET
> > > - check for !audit_task_info, throw -ENOPROTOOPT on set
> > > - changed -EPERM to -EEXIST for parent check
> > > - return AUDIT_CID_UNSET if !audit_enabled
> > > - squash child/thread check patch into AUDIT_CONTAINER_ID patch
> > > - changed -EPERM to -EBUSY for child check
> > > - separate child and thread checks, use -EALREADY for latter
> > > - move addition of op= from ptrace/signal patch to AUDIT_CONTAINER patch
> > > - fix && to || bashism in ptrace/signal patch
> > > - uninline and export function for audit_free_context()
> > > - drop CONFIG_CHANGE, FEATURE_CHANGE, ANOM_ABEND, ANOM_SECCOMP patches
> > > - move audit_enabled check (xt_AUDIT)
> > > - switched from containerid list in struct net to net_generic's struct audit_net
> > > - move containerid list iteration into audit (xt_AUDIT)
> > > - create function to move namespace switch into audit
> > > - switched /proc/PID/ entry from containerid to audit_containerid
> > > - call kzalloc with GFP_ATOMIC on in_atomic() in audit_alloc_context()
> > > - call kzalloc with GFP_ATOMIC on in_atomic() in audit_log_container_info()
> > > - use xt_net(par) instead of sock_net(skb->sk) to get net
> > > - switched record and field names: initial CONTAINER_ID, aux CONTAINER, field CONTID
> > > - allow to set own contid
> > > - open code audit_set_containerid
> > > - add contid inherited flag
> > > - ccontainerid and pcontainerid eliminated due to inherited flag
> > > - change name of container list funcitons
> > > - rename containerid to contid
> > > - convert initial container record to syscall aux
> > > - fix spelling mistake of contidion in net/rfkill/core.c to avoid contid name collision
> > >
> > > v2
> > > - add check for children and threads
> > > - add network namespace container identifier list
> > > - add NETFILTER_PKT audit container identifier logging
> > > - patch description and documentation clean-up and example
> > > - reap unused ppid
> > >
> > > Richard Guy Briggs (10):
> > > audit: collect audit task parameters
> > > audit: add container id
> > > audit: log container info of syscalls
> > > audit: add containerid support for ptrace and signals
> > > audit: add support for non-syscall auxiliary records
> > > audit: add containerid support for tty_audit
> > > audit: add containerid filtering
> > > audit: add support for containerid to network namespaces
> > > audit: NETFILTER_PKT: record each container ID associated with a netNS
> > > debug audit: read container ID of a process
> > >
> > > drivers/tty/tty_audit.c | 5 +-
> > > fs/proc/base.c | 56 ++++++++++++++
> > > include/linux/audit.h | 95 ++++++++++++++++++++---
> > > include/linux/sched.h | 5 +-
> > > include/uapi/linux/audit.h | 8 +-
> > > init/init_task.c | 3 +-
> > > init/main.c | 2 +
> > > kernel/audit.c | 137 +++++++++++++++++++++++++++++++++
> > > kernel/audit.h | 4 +
> > > kernel/auditfilter.c | 47 ++++++++++++
> > > kernel/auditsc.c | 183 ++++++++++++++++++++++++++++++++++++++++-----
> > > kernel/fork.c | 4 +-
> > > kernel/nsproxy.c | 4 +
> > > net/netfilter/xt_AUDIT.c | 12 ++-
> > > 14 files changed, 526 insertions(+), 39 deletions(-)
> > >
> > > --
> > > 1.8.3.1
> > >
>
> - 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
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox