On Sat, Aug 27, 2016 at 09:55:01PM +0200, Mickaël Salaün wrote:
>
> On 27/08/2016 20:19, Alexei Starovoitov wrote:
> > On Sat, Aug 27, 2016 at 04:34:55PM +0200, Mickaël Salaün wrote:
> >>
> >> On 27/08/2016 01:05, Alexei Starovoitov wrote:
> >>> On Fri, Aug 26, 2016 at 05:10:40PM +0200, Mickaël Salaün wrote:
> >>>
> >>>>> As far as safety and type checking that bpf programs has to do,
> >>>>> I like the approach of patch 06/10:
> >>>>> +LANDLOCK_HOOK2(file_open, FILE_OPEN,
> >>>>> + PTR_TO_STRUCT_FILE, struct file *, file,
> >>>>> + PTR_TO_STRUCT_CRED, const struct cred *, cred
> >>>>> +)
> >>>>> teaching verifier to recognize struct file, cred, sockaddr
> >>>>> will let bpf program access them naturally without any overhead.
> >>>>> Though:
> >>>>> @@ -102,6 +102,9 @@ enum bpf_prog_type {
> >>>>> BPF_PROG_TYPE_SCHED_CLS,
> >>>>> BPF_PROG_TYPE_SCHED_ACT,
> >>>>> BPF_PROG_TYPE_TRACEPOINT,
> >>>>> + BPF_PROG_TYPE_LANDLOCK_FILE_OPEN,
> >>>>> + BPF_PROG_TYPE_LANDLOCK_FILE_PERMISSION,
> >>>>> + BPF_PROG_TYPE_LANDLOCK_MMAP_FILE,
> >>>>> };
> >>>>> is a bit of overkill.
> >>>>> I think it would be cleaner to have single
> >>>>> BPF_PROG_TYPE_LSM and at program load time pass
> >>>>> lsm_hook_id as well, so that verifier can do safety checks
> >>>>> based on type info provided in LANDLOCK_HOOKs
> >>>>
> >>>> I first started with a unique BPF_PROG_TYPE but, the thing is, the BPF
> >>>> verifier check programs according to their types. If we need to check
> >>>> specific context value types (e.g. PTR_TO_STRUCT_FILE), we need a
> >>>> dedicated program types. I don't see any other way to do it with the
> >>>> current verifier code. Moreover it's the purpose of program types, right?
> >>>
> >>> Adding new bpf program type for every lsm hook is not acceptable.
> >>> Either do one new program type + pass lsm_hook_id as suggested
> >>> or please come up with an alternative approach.
> >>
> >> OK, so we have to modify the verifier to not only rely on the program
> >> type but on another value to check the context accesses. Do you have a
> >> hint from where this value could come from? Do we need to add a new bpf
> >> command to associate a program to a subtype?
> >
> > It's another field prog_subtype (or prog_hook_id) in union bpf_attr.
> > Both prog_type and prog_hook_id are used during verification.
> > prog_type distinguishes the main aspects whereas prog_hook_id selects
> > which lsm_hook's argument definition to apply.
> > At the time of attaching to a hook, the prog_hook_id passed at the
> > load time should match lsm's hook_id.
>
> OK, so this new prog_subtype field should be use/set by a new bpf_cmd,
> right? Something like BPF_PROG_SUBTYPE or BPF_PROG_METADATA?
No new command. It will be an optional field to existing BPF_PROG_LOAD.
In other words instead of replicating everything that different
bpf_prog_type-s need, we can pass this hook_id field to fine tune
the purpose (and applicability) of the program.
And one BPF_PROG_TYPE_LANDLOCK type will cover all lsm hooks.
For example existing BPF_PROG_TYPE_TRACEPOINT checks the safety
for all tracepoints while they all have different arguments.
For tracepoints it's easier, since the only difference between them
is the range of ctx access. Here we need strong type safety
of arguments therefore need extra hook_id at load time.
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
