Re: [RFC PATCH v2 1/1] audit: Add generic compat syscall support

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

Will, I always thank you for your comments:

On 11/26/2013 04:01 AM, Will Deacon wrote:
> On Tue, Nov 19, 2013 at 09:43:55AM +0000, AKASHI Takahiro wrote:
>> (v1 was created mistakenly. Please igore it.)
>>
>> lib/audit.c provides a generic definition for auditing system calls.
>> lib/compat_audit.c similarly adds compat syscall support for
>> bi-architectures (32/64-bit).
>>
>> Each architecture 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
>>
>> diff --git a/include/linux/audit.h b/include/linux/audit.h
>> index 729a4d1..c49a312 100644
>> --- a/include/linux/audit.h
>> +++ b/include/linux/audit.h
>> @@ -76,6 +76,15 @@ struct audit_field {
>>   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 f3bb2cb..5bb185a 100644
>> --- a/lib/Makefile
>> +++ b/lib/Makefile
>> @@ -96,6 +96,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..3bf3858 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
>
> Hmm, I'm not sure this is the right way to solve this problem. Whether
> something is compat or not depends on the task to which it is associated. If
> this is always the current task for the audit cases, then you can just use
> something like is_compat_task. Otherwise, I think we need to get a handle on
> the task_struct here. An arch-callback feels like the wrong approach to me.

You are completely right. In my current (v3 prototype) implementation,
"abi" argument, which can be AUDIT_ARCH_ARM(EB) or AUDIT_ARCH_AARCH64(EB),
passed to audit_classify_syscall() is determined per-task using is_compat_thread()
when audit_syscall_entry() is executed in syscall_trace().
(Obviously audit_is_compat() is true only in case of AUDIT_ARCH_ARM.)

V3 based on this patch is working for 32-bit and 64-bit userland.
I can submit v3 patch if you want.

Thanks,
-Takahiro AKASHI

> Will
>