Re: [PATCH -next] cred: conditionally declare groups-related functions
On Thu, Jun 21, 2018 at 4:33 AM Ondrej Mosnacek wrote:
>
> The groups-related functions declared in include/linux/cred.h are
> defined in kernel/groups.c, which is compiled only when
> CONFIG_MULTIUSER=y. Move all these function declarations under #ifdef
> CONFIG_MULTIUSER to help avoid accidental usage in contexts where
> CONFIG_MULTIUSER might be disabled.
>
> This patch also adds a fallback for groups_search(). Currently this
> function is only called from kernel/groups.c itself and
> keys/permissions.c, which depends on CONFIG_MULTIUSER. However, the
> audit subsystem (which does not depend on CONFIG_MULTIUSER) calls this
> function in -next, so the fallback will be needed to avoid compilation
> errors or ugly workarounds.
>
> See also:
> https://lkml.org/lkml/2018/6/20/670
> https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/audit.git/commit/?h=next&id=af85d1772e31fed34165a1b3decef340cf4080c0
>
> Reported-by: Randy Dunlap
> Signed-off-by: Ondrej Mosnacek
> ---
> include/linux/cred.h | 16 +++-
> 1 file changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/include/linux/cred.h b/include/linux/cred.h
> index 631286535d0f..8917768453cc 100644
> --- a/include/linux/cred.h
> +++ b/include/linux/cred.h
> @@ -65,6 +65,12 @@ extern void groups_free(struct group_info *);
>
> extern int in_group_p(kgid_t);
> extern int in_egroup_p(kgid_t);
> +
> +extern int set_current_groups(struct group_info *);
> +extern void set_groups(struct cred *, struct group_info *);
> +extern int groups_search(const struct group_info *, kgid_t);
> +extern bool may_setgroups(void);
> +extern void groups_sort(struct group_info *);
> #else
> static inline void groups_free(struct group_info *group_info)
> {
> @@ -78,12 +84,12 @@ static inline int in_egroup_p(kgid_t grp)
> {
> return 1;
> }
> +
> +static inline int groups_search(const struct group_info *group_info, kgid_t
> grp)
> +{
> + return 0;
Is this the right fallback value? If CONFIG_MULTIUSER is disabled,
wouldn't we always want to indicate a group match? The in_group_p()
and in_egroup_p() dummy functions would seem to indicate that is the
correct behavior ...
> +}
> #endif
> -extern int set_current_groups(struct group_info *);
> -extern void set_groups(struct cred *, struct group_info *);
> -extern int groups_search(const struct group_info *, kgid_t);
> -extern bool may_setgroups(void);
> -extern void groups_sort(struct group_info *);
>
> /*
> * The security context of a task
> --
> 2.17.1
>
--
paul moore
www.paul-moore.com
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH -next] cred: conditionally declare groups-related functions
On 06/21/2018 01:33 AM, Ondrej Mosnacek wrote: > The groups-related functions declared in include/linux/cred.h are > defined in kernel/groups.c, which is compiled only when > CONFIG_MULTIUSER=y. Move all these function declarations under #ifdef > CONFIG_MULTIUSER to help avoid accidental usage in contexts where > CONFIG_MULTIUSER might be disabled. > > This patch also adds a fallback for groups_search(). Currently this > function is only called from kernel/groups.c itself and > keys/permissions.c, which depends on CONFIG_MULTIUSER. However, the > audit subsystem (which does not depend on CONFIG_MULTIUSER) calls this > function in -next, so the fallback will be needed to avoid compilation > errors or ugly workarounds. > > See also: > https://lkml.org/lkml/2018/6/20/670 > https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/audit.git/commit/?h=next&id=af85d1772e31fed34165a1b3decef340cf4080c0 > > Reported-by: Randy Dunlap > Signed-off-by: Ondrej Mosnacek Tested-by: Randy Dunlap Thanks. > --- > include/linux/cred.h | 16 +++- > 1 file changed, 11 insertions(+), 5 deletions(-) -- ~Randy -- Linux-audit mailing list Linux-audit@redhat.com https://www.redhat.com/mailman/listinfo/linux-audit
Re: [PATCH] kernel: audit_tree: Fix a sleep-in-atomic-context bug
On Thu, Jun 21, 2018 at 11:32:45AM +0800, Jia-Ju Bai wrote:
> The kernel may sleep with holding a spinlock.
> The function call paths (from bottom to top) in Linux-4.16.7 are:
>
> [FUNC] kmem_cache_alloc(GFP_KERNEL)
> fs/notify/mark.c, 439:
> kmem_cache_alloc in fsnotify_attach_connector_to_object
> fs/notify/mark.c, 520:
> fsnotify_attach_connector_to_object in fsnotify_add_mark_list
> fs/notify/mark.c, 590:
> fsnotify_add_mark_list in fsnotify_add_mark_locked
> kernel/audit_tree.c, 437:
> fsnotify_add_mark_locked in tag_chunk
> kernel/audit_tree.c, 423:
> spin_lock in tag_chunk
There are several locks here; your report would be improved by saying
which one is the problem. I'm assuming it's old_entry->lock.
spin_lock(&old_entry->lock);
...
if (fsnotify_add_inode_mark_locked(chunk_entry,
old_entry->connector->inode, 1)) {
...
return fsnotify_add_mark_locked(mark, inode, NULL, allow_dups);
...
ret = fsnotify_add_mark_list(mark, inode, mnt, allow_dups);
...
if (inode)
connp = &inode->i_fsnotify_marks;
conn = fsnotify_grab_connector(connp);
if (!conn) {
err = fsnotify_attach_connector_to_object(connp, inode, mnt);
It seems to me that this is safe because old_entry is looked up from
fsnotify_find_mark, and it can't be removed while its lock is held.
Therefore there's always a 'conn' returned from fsnotify_grab_connector(),
and so this path will never be taken.
But this code path is confusing to me, and I could be wrong. Jan, please
confirm my analysis is correct?
> [FUNC] kmem_cache_alloc(GFP_KERNEL)
> fs/notify/mark.c, 439:
> kmem_cache_alloc in fsnotify_attach_connector_to_object
> fs/notify/mark.c, 520:
> fsnotify_attach_connector_to_object in fsnotify_add_mark_list
> fs/notify/mark.c, 590:
> fsnotify_add_mark_list in fsnotify_add_mark_locked
> kernel/audit_tree.c, 291:
> fsnotify_add_mark_locked in untag_chunk
> kernel/audit_tree.c, 258:
> spin_lock in untag_chunk
I'm just going to assume this one is the same.
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
[PATCH] kernel: audit_tree: Fix a sleep-in-atomic-context bug
The kernel may sleep with holding a spinlock.
The function call paths (from bottom to top) in Linux-4.16.7 are:
[FUNC] kmem_cache_alloc(GFP_KERNEL)
fs/notify/mark.c, 439:
kmem_cache_alloc in fsnotify_attach_connector_to_object
fs/notify/mark.c, 520:
fsnotify_attach_connector_to_object in fsnotify_add_mark_list
fs/notify/mark.c, 590:
fsnotify_add_mark_list in fsnotify_add_mark_locked
kernel/audit_tree.c, 437:
fsnotify_add_mark_locked in tag_chunk
kernel/audit_tree.c, 423:
spin_lock in tag_chunk
[FUNC] kmem_cache_alloc(GFP_KERNEL)
fs/notify/mark.c, 439:
kmem_cache_alloc in fsnotify_attach_connector_to_object
fs/notify/mark.c, 520:
fsnotify_attach_connector_to_object in fsnotify_add_mark_list
fs/notify/mark.c, 590:
fsnotify_add_mark_list in fsnotify_add_mark_locked
kernel/audit_tree.c, 291:
fsnotify_add_mark_locked in untag_chunk
kernel/audit_tree.c, 258:
spin_lock in untag_chunk
To fix this bug, GFP_KERNEL is replaced with GFP_ATOMIC.
This bug is found by my static analysis tool (DSAC-2) and checked by my
code review.
Signed-off-by: Jia-Ju Bai
---
fs/notify/mark.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index e9191b416434..c664853b8585 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -436,7 +436,7 @@ static int fsnotify_attach_connector_to_object(
{
struct fsnotify_mark_connector *conn;
- conn = kmem_cache_alloc(fsnotify_mark_connector_cachep, GFP_KERNEL);
+ conn = kmem_cache_alloc(fsnotify_mark_connector_cachep, GFP_ATOMIC);
if (!conn)
return -ENOMEM;
spin_lock_init(&conn->lock);
--
2.17.0
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: [RFC PATCH ghak10 2/3] audit: Add the audit_adjtime() function
ut 19. 6. 2018 o 22:52 Steve Grubb napísal(a):
> On Tuesday, June 19, 2018 4:57:37 AM EDT Ondrej Mosnacek wrote:
> > 2018-06-18 20:39 GMT+02:00 Steve Grubb :
> > > On Friday, June 15, 2018 8:45:22 AM EDT Ondrej Mosnacek wrote:
> > >> This patch adds a new function that shall be used to log any
> > >> modification of the system's clock (via the adjtimex() syscall).
> > >>
> > >> The function logs an audit record of type AUDIT_TIME_ADJUSTED with the
> > >> following fields:
> > >> * txmodes (the 'modes' field of struct timex)
> > >> * txoffset (the 'offset' field of struct timex)
> > >> * txfreq (the 'freq' field of struct timex)
> > >> * txmaxerr (the 'maxerror' field of struct timex)
> > >> * txesterr (the 'esterror' field of struct timex)
> > >> * txstatus (the 'status' field of struct timex)
> > >> * txconst (the 'constant' field of struct timex)
> > >> * txsec, txusec (the 'tv_sec' and 'tv_usec' fields of the 'time' field
> > >> of struct timex (respectively))
> > >> * txtick (the 'tick' field of struct timex)
> > >
> > > Are all of these fields security relevant? Primarily what we need to know
> > > is if time is being adjusted. This is relevant because a bad guy may
> > > adjust system time to make something appear to happen earlier or later
> > > than it really did which make correlation hard or impossible.
> >
> > This is still an open question for me... On the one hand, we might
> > want to know exactly what the bad guy was trying to do ("He changed
> > the offset to 500 ms." vs. just "He adjusted the clock."), on the
> > other hand, we may not really care and consider it yet another junk
> > data in the logs... A possible compromise could be to log only
> > relevant fields (see 'Possible improvements' in the commit message).
> > Assuming ntpd or other authorized applications would only modify
> > one/few variables at a time, this would add only a few fields to the
> > record each time.
>
> I think we want the modes field so that we know what was changed. But do we
> really need to know maxerror or status? I think we should limit this to the
> modes and any value of a time adjustment.
Well, the status field can be at the very least used to set the
STA_INS/STA_DEL status flags [1], which seem to coordinate the
insertion of leap second. Now this may be a bit far-fetched, but I can
imagine how bogus insertions of leap seconds could be used to
gradually shift the clock to a bad value. I think we can safely drop
maxerror/esterror (I believe these are just informational of how much
the clock is 'out of sync'), but the rest seems like it could at least
indirectly influence the time (IOW, I wasn't able to find a proof that
it can't...).
[1]
https://elixir.bootlin.com/linux/v4.17/source/include/uapi/linux/timex.h#L128
>
> > Note that this new auxiliary record gets only logged on *modifying*
> > operations, which should not be that frequent, and thus it shouldn't
> > be a problem to output a bit of potentially useful information.
>
> We're after just security information.
>
> > That said, I don't mind logging just an empty record if that is
> > preferred.
>
> An empty buffer doesn't tell us what was adjusted.
Yes, sorry, that should have been "a record with only modes field".
>
> Thanks,
> -Steve
>
>--
Ondrej Mosnacek
Associate Software Engineer, Security Technologies
Red Hat, Inc.
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
[PATCH -next] cred: conditionally declare groups-related functions
The groups-related functions declared in include/linux/cred.h are
defined in kernel/groups.c, which is compiled only when
CONFIG_MULTIUSER=y. Move all these function declarations under #ifdef
CONFIG_MULTIUSER to help avoid accidental usage in contexts where
CONFIG_MULTIUSER might be disabled.
This patch also adds a fallback for groups_search(). Currently this
function is only called from kernel/groups.c itself and
keys/permissions.c, which depends on CONFIG_MULTIUSER. However, the
audit subsystem (which does not depend on CONFIG_MULTIUSER) calls this
function in -next, so the fallback will be needed to avoid compilation
errors or ugly workarounds.
See also:
https://lkml.org/lkml/2018/6/20/670
https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/audit.git/commit/?h=next&id=af85d1772e31fed34165a1b3decef340cf4080c0
Reported-by: Randy Dunlap
Signed-off-by: Ondrej Mosnacek
---
include/linux/cred.h | 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/include/linux/cred.h b/include/linux/cred.h
index 631286535d0f..8917768453cc 100644
--- a/include/linux/cred.h
+++ b/include/linux/cred.h
@@ -65,6 +65,12 @@ extern void groups_free(struct group_info *);
extern int in_group_p(kgid_t);
extern int in_egroup_p(kgid_t);
+
+extern int set_current_groups(struct group_info *);
+extern void set_groups(struct cred *, struct group_info *);
+extern int groups_search(const struct group_info *, kgid_t);
+extern bool may_setgroups(void);
+extern void groups_sort(struct group_info *);
#else
static inline void groups_free(struct group_info *group_info)
{
@@ -78,12 +84,12 @@ static inline int in_egroup_p(kgid_t grp)
{
return 1;
}
+
+static inline int groups_search(const struct group_info *group_info, kgid_t
grp)
+{
+ return 0;
+}
#endif
-extern int set_current_groups(struct group_info *);
-extern void set_groups(struct cred *, struct group_info *);
-extern int groups_search(const struct group_info *, kgid_t);
-extern bool may_setgroups(void);
-extern void groups_sort(struct group_info *);
/*
* The security context of a task
--
2.17.1
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
Re: linux-next: Tree for Jun 20 (kernel/auditsc.c)
st 20. 6. 2018 o 22:00 Paul Moore napísal(a):
>
> On Wed, Jun 20, 2018 at 11:48 AM Randy Dunlap wrote:
> >
> > On 06/19/2018 09:42 PM, Stephen Rothwell wrote:
> > > Hi all,
> > >
> > > Changes since 20180619:
> > >
> >
> > on x86_64:
> >
> > kernel/auditsc.o: In function `audit_filter_rules.isra.20':
> > auditsc.c:(.text+0x8c5): undefined reference to `groups_search'
> > auditsc.c:(.text+0x909): undefined reference to `groups_search'
> >
> >
> > Reported-by: Randy Dunlap
> >
> > Full randconfig file is attached.
>
> Thanks Randy.
>
> I believe the issue is that when CONFIG_MULTIUSER is not enabled
> groups.{c.o} is not included. Ondrej, please look into this and
> submit a fix as soon as possible, thank you.
I'm working on it, stay tuned... It should be an easy fix, but it will
(likely) need changes outside the audit code. We could work around it
locally, of course, but since this is just in -next, I believe it's
worth to fix it properly.
Cheers,
--
Ondrej Mosnacek
Associate Software Engineer, Security Technologies
Red Hat, Inc.
--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit
