| Message ID | 1389945795-4255-2-git-send-email-takahiro.akashi@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Fri, Jan 17, 2014 at 08:03:15AM +0000, AKASHI Takahiro wrote: > lib/audit.c provides a generic definition for auditing system calls. > This patch extends it for compat syscall support on bi-architectures > (32/64-bit) by adding lib/compat_audit.c when CONFIG_COMPAT enabled. > > Each architecture that wants to use this must define audit_is_compat() > in asm/audit.h. > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> I'm not familiar with the audit subsystem but I have some (cosmetic) comments below. > diff --git a/include/linux/audit.h b/include/linux/audit.h > index bf1ef22..3d71949 100644 > --- a/include/linux/audit.h > +++ b/include/linux/audit.h > @@ -78,6 +78,15 @@ extern int is_audit_feature_set(int which); > extern int __init audit_register_class(int class, unsigned *list); > extern int audit_classify_syscall(int abi, unsigned syscall); > extern int audit_classify_arch(int arch); > +#if defined(CONFIG_AUDIT_GENERIC) && defined(CONFIG_COMPAT) > +extern unsigned compat_write_class[]; > +extern unsigned compat_read_class[]; > +extern unsigned compat_dir_class[]; > +extern unsigned compat_chattr_class[]; > +extern unsigned compat_signal_class[]; > + > +extern int audit_classify_compat_syscall(int abi, unsigned syscall); > +#endif > > /* audit_names->type values */ > #define AUDIT_TYPE_UNKNOWN 0 /* we don't know yet */ > diff --git a/lib/Makefile b/lib/Makefile > index a459c31..73ea908 100644 > --- a/lib/Makefile > +++ b/lib/Makefile > @@ -93,6 +93,9 @@ obj-$(CONFIG_TEXTSEARCH_BM) += ts_bm.o > obj-$(CONFIG_TEXTSEARCH_FSM) += ts_fsm.o > obj-$(CONFIG_SMP) += percpu_counter.o > obj-$(CONFIG_AUDIT_GENERIC) += audit.o > +ifeq ($(CONFIG_COMPAT),y) > +obj-$(CONFIG_AUDIT_GENERIC) += compat_audit.o > +endif You could use a CONFIG_AUDIT_COMPAT_GENERIC and simplify other #ifdefs as well. > --- a/lib/audit.c > +++ b/lib/audit.c > @@ -1,6 +1,7 @@ > #include <linux/init.h> > #include <linux/types.h> > #include <linux/audit.h> > +#include <asm/audit.h> > #include <asm/unistd.h> > > static unsigned dir_class[] = { > @@ -30,11 +31,20 @@ static unsigned signal_class[] = { > > int audit_classify_arch(int arch) > { > +#ifdef CONFIG_COMPAT > + if (audit_is_compat(arch)) > + return 1; > +#endif > return 0; > } Here and in other places, just define a default audit_is_compat() functions which returns false when !CONFIG_COMPAT to avoid the #ifdefs. > diff --git a/lib/compat_audit.c b/lib/compat_audit.c > new file mode 100644 > index 0000000..94f6480 > --- /dev/null > +++ b/lib/compat_audit.c > @@ -0,0 +1,51 @@ > +#include <linux/init.h> > +#include <linux/types.h> > +/* FIXME: this might be architecture dependent */ > +#include <asm/unistd_32.h> It most likely is architecture dependent. > +int audit_classify_compat_syscall(int abi, unsigned syscall) > +{ > + switch (syscall) { > +#ifdef __NR_open > + case __NR_open: > + return 2; > +#endif > +#ifdef __NR_openat > + case __NR_openat: > + return 3; > +#endif > +#ifdef __NR_socketcall > + case __NR_socketcall: > + return 4; > +#endif > + case __NR_execve: > + return 5; > + default: > + return 1; > + } > +} BTW, since they aren't many, you could get the arch code to define __NR_compat_open etc. explicitly and use these. On arm64 we have a few of these defined to avoid name collision in signal handling code.
Catalin and audit maintainers, On 01/23/2014 11:51 PM, Catalin Marinas wrote: > On Fri, Jan 17, 2014 at 08:03:15AM +0000, AKASHI Takahiro wrote: >> lib/audit.c provides a generic definition for auditing system calls. >> This patch extends it for compat syscall support on bi-architectures >> (32/64-bit) by adding lib/compat_audit.c when CONFIG_COMPAT enabled. >> >> Each architecture that wants to use this must define audit_is_compat() >> in asm/audit.h. >> >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > > I'm not familiar with the audit subsystem but I have some (cosmetic) > comments below. > >> diff --git a/include/linux/audit.h b/include/linux/audit.h >> index bf1ef22..3d71949 100644 >> --- a/include/linux/audit.h >> +++ b/include/linux/audit.h >> @@ -78,6 +78,15 @@ extern int is_audit_feature_set(int which); >> extern int __init audit_register_class(int class, unsigned *list); >> extern int audit_classify_syscall(int abi, unsigned syscall); >> extern int audit_classify_arch(int arch); >> +#if defined(CONFIG_AUDIT_GENERIC) && defined(CONFIG_COMPAT) >> +extern unsigned compat_write_class[]; >> +extern unsigned compat_read_class[]; >> +extern unsigned compat_dir_class[]; >> +extern unsigned compat_chattr_class[]; >> +extern unsigned compat_signal_class[]; >> + >> +extern int audit_classify_compat_syscall(int abi, unsigned syscall); >> +#endif >> >> /* audit_names->type values */ >> #define AUDIT_TYPE_UNKNOWN 0 /* we don't know yet */ >> diff --git a/lib/Makefile b/lib/Makefile >> index a459c31..73ea908 100644 >> --- a/lib/Makefile >> +++ b/lib/Makefile >> @@ -93,6 +93,9 @@ obj-$(CONFIG_TEXTSEARCH_BM) += ts_bm.o >> obj-$(CONFIG_TEXTSEARCH_FSM) += ts_fsm.o >> obj-$(CONFIG_SMP) += percpu_counter.o >> obj-$(CONFIG_AUDIT_GENERIC) += audit.o >> +ifeq ($(CONFIG_COMPAT),y) >> +obj-$(CONFIG_AUDIT_GENERIC) += compat_audit.o >> +endif > > You could use a CONFIG_AUDIT_COMPAT_GENERIC and simplify other #ifdefs > as well. I will add the following in lib/Kconfig: config AUDIT_COMPAT_GENERIC depends on AUDIT_GENERIC & COMPAT default y >> --- a/lib/audit.c >> +++ b/lib/audit.c >> @@ -1,6 +1,7 @@ >> #include <linux/init.h> >> #include <linux/types.h> >> #include <linux/audit.h> >> +#include <asm/audit.h> >> #include <asm/unistd.h> >> >> static unsigned dir_class[] = { >> @@ -30,11 +31,20 @@ static unsigned signal_class[] = { >> >> int audit_classify_arch(int arch) >> { >> +#ifdef CONFIG_COMPAT >> + if (audit_is_compat(arch)) >> + return 1; >> +#endif >> return 0; >> } > > Here and in other places, just define a default audit_is_compat() > functions which returns false when !CONFIG_COMPAT to avoid the #ifdefs. OK. With Richard's comment, the definition below will be added to uapi/linux/audit.h: #ifdef CONFIG_COMPAT #define audit_is_compat(arch) (!!((arch) & __AUDIT_ARCH_64BIT)) #else #define audit_is_compat(arch) false #endif >> diff --git a/lib/compat_audit.c b/lib/compat_audit.c >> new file mode 100644 >> index 0000000..94f6480 >> --- /dev/null >> +++ b/lib/compat_audit.c >> @@ -0,0 +1,51 @@ >> +#include <linux/init.h> >> +#include <linux/types.h> >> +/* FIXME: this might be architecture dependent */ >> +#include <asm/unistd_32.h> > > It most likely is architecture dependent. I'm wondering what name is the most appropriate in this case. Most archictures have __NR_xyz definitions in "unistd_32.h", but arm64 doesn't have it, instead "unistd32." which contains only __SYSCALL(xyz, NO). Confusing? >> +int audit_classify_compat_syscall(int abi, unsigned syscall) >> +{ >> + switch (syscall) { >> +#ifdef __NR_open >> + case __NR_open: >> + return 2; >> +#endif >> +#ifdef __NR_openat >> + case __NR_openat: >> + return 3; >> +#endif >> +#ifdef __NR_socketcall >> + case __NR_socketcall: >> + return 4; >> +#endif >> + case __NR_execve: >> + return 5; >> + default: >> + return 1; >> + } >> +} > > BTW, since they aren't many, you could get the arch code to define > __NR_compat_open etc. explicitly and use these. On arm64 we have a few > of these defined to avoid name collision in signal handling code. Again, most architecture have their own unistd32.h for compat system calls, and use __NR_open-like naming. It's unlikely for these archs to migrate to "generic compat" auditing, but I believe that '__NR_open'-like naming is better because we may be able to avoid arch-specific changes even for future(?) syscall-related enhancements in audit. But, anyway, it's up to audit maintainer's preference. Thanks, -Takahiro AKASHI
On Mon, Jan 27, 2014 at 05:58:07AM +0000, AKASHI Takahiro wrote: > Catalin and audit maintainers, > > On 01/23/2014 11:51 PM, Catalin Marinas wrote: > > On Fri, Jan 17, 2014 at 08:03:15AM +0000, AKASHI Takahiro wrote: > >> diff --git a/lib/compat_audit.c b/lib/compat_audit.c > >> new file mode 100644 > >> index 0000000..94f6480 > >> --- /dev/null > >> +++ b/lib/compat_audit.c > >> @@ -0,0 +1,51 @@ > >> +#include <linux/init.h> > >> +#include <linux/types.h> > >> +/* FIXME: this might be architecture dependent */ > >> +#include <asm/unistd_32.h> > > > > It most likely is architecture dependent. > > I'm wondering what name is the most appropriate in this case. > Most archictures have __NR_xyz definitions in "unistd_32.h", > but arm64 doesn't have it, instead "unistd32." which contains > only __SYSCALL(xyz, NO). Confusing? I don't think we should introduce a new file (or at least it should be named something containing "audit" to make it clearer). > >> +int audit_classify_compat_syscall(int abi, unsigned syscall) > >> +{ > >> + switch (syscall) { > >> +#ifdef __NR_open > >> + case __NR_open: > >> + return 2; > >> +#endif > >> +#ifdef __NR_openat > >> + case __NR_openat: > >> + return 3; > >> +#endif > >> +#ifdef __NR_socketcall > >> + case __NR_socketcall: > >> + return 4; > >> +#endif > >> + case __NR_execve: > >> + return 5; > >> + default: > >> + return 1; > >> + } > >> +} > > > > BTW, since they aren't many, you could get the arch code to define > > __NR_compat_open etc. explicitly and use these. On arm64 we have a few > > of these defined to avoid name collision in signal handling code. > > Again, most architecture have their own unistd32.h for compat system calls, > and use __NR_open-like naming. > It's unlikely for these archs to migrate to "generic compat" auditing, > but I believe that '__NR_open'-like naming is better because we may be able to avoid > arch-specific changes even for future(?) syscall-related enhancements in audit. My preference is as above, a few __NR_compat_* (just those required by audit) defined in unistd.h but I'm not an audit maintainer.
Catalin, Let me correct myself, On 01/27/2014 09:15 PM, Catalin Marinas wrote: > On Mon, Jan 27, 2014 at 05:58:07AM +0000, AKASHI Takahiro wrote: >> Catalin and audit maintainers, >> >> On 01/23/2014 11:51 PM, Catalin Marinas wrote: >>> On Fri, Jan 17, 2014 at 08:03:15AM +0000, AKASHI Takahiro wrote: >>>> diff --git a/lib/compat_audit.c b/lib/compat_audit.c >>>> new file mode 100644 >>>> index 0000000..94f6480 >>>> --- /dev/null >>>> +++ b/lib/compat_audit.c >>>> @@ -0,0 +1,51 @@ >>>> +#include <linux/init.h> >>>> +#include <linux/types.h> >>>> +/* FIXME: this might be architecture dependent */ >>>> +#include <asm/unistd_32.h> >>> >>> It most likely is architecture dependent. >> >> I'm wondering what name is the most appropriate in this case. >> Most archictures have __NR_xyz definitions in "unistd_32.h", >> but arm64 doesn't have it, instead "unistd32." which contains >> only __SYSCALL(xyz, NO). Confusing? > > I don't think we should introduce a new file (or at least it should be > named something containing "audit" to make it clearer). > >>>> +int audit_classify_compat_syscall(int abi, unsigned syscall) >>>> +{ >>>> + switch (syscall) { >>>> +#ifdef __NR_open >>>> + case __NR_open: >>>> + return 2; >>>> +#endif >>>> +#ifdef __NR_openat >>>> + case __NR_openat: >>>> + return 3; >>>> +#endif >>>> +#ifdef __NR_socketcall >>>> + case __NR_socketcall: >>>> + return 4; >>>> +#endif >>>> + case __NR_execve: >>>> + return 5; >>>> + default: >>>> + return 1; >>>> + } >>>> +} >>> >>> BTW, since they aren't many, you could get the arch code to define >>> __NR_compat_open etc. explicitly and use these. On arm64 we have a few >>> of these defined to avoid name collision in signal handling code. >> >> Again, most architecture have their own unistd32.h for compat system calls, >> and use __NR_open-like naming. >> It's unlikely for these archs to migrate to "generic compat" auditing, >> but I believe that '__NR_open'-like naming is better because we may be able to avoid >> arch-specific changes even for future(?) syscall-related enhancements in audit. In my compat_audit.c, all the entries in audit classes are derived from asm-generic/audit_*.h, where __NR_xyz are used to list the system calls. So it is not possible to use __NR_compat_xyz as far as we re-use those generic files. (Obviously we don't want to duplicate those header files, that is, audit_compat_*.h.) I agree that we should not have similar but different files, like unist32.h and unistd_32.h, but it seems to be inevitable for our case. (That is the reason why I dynamically generate unistd_32.h) As for arch-specific header file name, "audit_unistd32.h" can be fine, but people on other architectures might be unhappy with such a name since they can commonly use unistd32.h instead. - Takahiro AKASHI > My preference is as above, a few __NR_compat_* (just those required by > audit) defined in unistd.h but I'm not an audit maintainer. >
diff --git a/include/linux/audit.h b/include/linux/audit.h index bf1ef22..3d71949 100644 --- a/include/linux/audit.h +++ b/include/linux/audit.h @@ -78,6 +78,15 @@ extern int is_audit_feature_set(int which); extern int __init audit_register_class(int class, unsigned *list); extern int audit_classify_syscall(int abi, unsigned syscall); extern int audit_classify_arch(int arch); +#if defined(CONFIG_AUDIT_GENERIC) && defined(CONFIG_COMPAT) +extern unsigned compat_write_class[]; +extern unsigned compat_read_class[]; +extern unsigned compat_dir_class[]; +extern unsigned compat_chattr_class[]; +extern unsigned compat_signal_class[]; + +extern int audit_classify_compat_syscall(int abi, unsigned syscall); +#endif /* audit_names->type values */ #define AUDIT_TYPE_UNKNOWN 0 /* we don't know yet */ diff --git a/lib/Makefile b/lib/Makefile index a459c31..73ea908 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -93,6 +93,9 @@ obj-$(CONFIG_TEXTSEARCH_BM) += ts_bm.o obj-$(CONFIG_TEXTSEARCH_FSM) += ts_fsm.o obj-$(CONFIG_SMP) += percpu_counter.o obj-$(CONFIG_AUDIT_GENERIC) += audit.o +ifeq ($(CONFIG_COMPAT),y) +obj-$(CONFIG_AUDIT_GENERIC) += compat_audit.o +endif obj-$(CONFIG_SWIOTLB) += swiotlb.o obj-$(CONFIG_IOMMU_HELPER) += iommu-helper.o diff --git a/lib/audit.c b/lib/audit.c index 76bbed4..e29ba82 100644 --- a/lib/audit.c +++ b/lib/audit.c @@ -1,6 +1,7 @@ #include <linux/init.h> #include <linux/types.h> #include <linux/audit.h> +#include <asm/audit.h> #include <asm/unistd.h> static unsigned dir_class[] = { @@ -30,11 +31,20 @@ static unsigned signal_class[] = { int audit_classify_arch(int arch) { +#ifdef CONFIG_COMPAT + if (audit_is_compat(arch)) + return 1; +#endif return 0; } int audit_classify_syscall(int abi, unsigned syscall) { +#ifdef CONFIG_COMPAT + if (audit_is_compat(abi)) + return audit_classify_compat_syscall(abi, syscall); +#endif + switch(syscall) { #ifdef __NR_open case __NR_open: @@ -57,6 +67,13 @@ int audit_classify_syscall(int abi, unsigned syscall) static int __init audit_classes_init(void) { +#ifdef CONFIG_COMPAT + audit_register_class(AUDIT_CLASS_WRITE_32, compat_write_class); + audit_register_class(AUDIT_CLASS_READ_32, compat_read_class); + audit_register_class(AUDIT_CLASS_DIR_WRITE_32, compat_dir_class); + audit_register_class(AUDIT_CLASS_CHATTR_32, compat_chattr_class); + audit_register_class(AUDIT_CLASS_SIGNAL_32, compat_signal_class); +#endif audit_register_class(AUDIT_CLASS_WRITE, write_class); audit_register_class(AUDIT_CLASS_READ, read_class); audit_register_class(AUDIT_CLASS_DIR_WRITE, dir_class); diff --git a/lib/compat_audit.c b/lib/compat_audit.c new file mode 100644 index 0000000..94f6480 --- /dev/null +++ b/lib/compat_audit.c @@ -0,0 +1,51 @@ +#include <linux/init.h> +#include <linux/types.h> +/* FIXME: this might be architecture dependent */ +#include <asm/unistd_32.h> + +unsigned compat_dir_class[] = { +#include <asm-generic/audit_dir_write.h> +~0U +}; + +unsigned compat_read_class[] = { +#include <asm-generic/audit_read.h> +~0U +}; + +unsigned compat_write_class[] = { +#include <asm-generic/audit_write.h> +~0U +}; + +unsigned compat_chattr_class[] = { +#include <asm-generic/audit_change_attr.h> +~0U +}; + +unsigned compat_signal_class[] = { +#include <asm-generic/audit_signal.h> +~0U +}; + +int audit_classify_compat_syscall(int abi, unsigned syscall) +{ + switch (syscall) { +#ifdef __NR_open + case __NR_open: + return 2; +#endif +#ifdef __NR_openat + case __NR_openat: + return 3; +#endif +#ifdef __NR_socketcall + case __NR_socketcall: + return 4; +#endif + case __NR_execve: + return 5; + default: + return 1; + } +}
lib/audit.c provides a generic definition for auditing system calls. This patch extends it for compat syscall support on bi-architectures (32/64-bit) by adding lib/compat_audit.c when CONFIG_COMPAT enabled. Each architecture that wants to use this must define audit_is_compat() in asm/audit.h. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- include/linux/audit.h | 9 +++++++++ lib/Makefile | 3 +++ lib/audit.c | 17 +++++++++++++++++ lib/compat_audit.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 80 insertions(+) create mode 100644 lib/compat_audit.c