netdev - Re: [PATCH bpf-next 1/6] bpf: introduce BPF_PROG_TYPE_FILE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20181006002218.amfazxko4sq32hu6@ast-mbp.dhcp.thefacebook.com>
Date:   Fri, 5 Oct 2018 17:22:20 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Al Viro <viro@...IV.linux.org.uk>
Cc:     Andy Lutomirski <luto@...capital.net>,
        Alexei Starovoitov <ast@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Daniel Borkmann <daniel@...earbox.net>,
        Network Development <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        kernel-team <kernel-team@...com>
Subject: Re: [PATCH bpf-next 1/6] bpf: introduce BPF_PROG_TYPE_FILE_FILTER

On Sat, Oct 06, 2018 at 12:47:06AM +0100, Al Viro wrote:
> 
> And no, it's not that each of those filesystems does not use inode->i_ino at all.
> There's a bunch of library helpers in fs/*.c that happen to use the value filesystem
> has seen fit to store there.  Whether to use those helpers or not, what to store in
> that field, etc. is, again, entirely up to the filesystem in question.
> generic_fillattr() is one of those, that's all there is to it.

makes sense.

> That's precisely why I really do not like the idea of hooks poking in the internals
> of kernel data structures.  Especially since not even "it's not visible outside of
> a subsystem-internal header" appears to slow you down.

that's why there is a code review to get the patches in shape that
don't expose kernel internals by _accident_.

> The same goes for tracepoints, etc. - turning random details of implementation into
> a carved in stone ABI is actively harmful.

we're not talking about tracepoints here.

> PS: If anything, visibility to hooks should be opt-in.

not sure how do you expect 'opt-in' to be imlemented.
As far as exposing inode and dev I agree that was a wrong approach.
Going to switch to struct file_handle instead.
Doing statx for every file_open is probably too slow for practical use.

> Sure, we can start actively hiding
> the things, but that's a winless arms race  - even if we bloody went and encrypted the
> private stuff, you'd still be able to pull decryption key from where it would be accessed
> by legitimate users, etc.  Нахуй нам это надо?

why do this ?
the use cases are explained in commit log.

Back to internals exposure.
Do you see an issue in the following:
struct bpf_file_info {
        __u32 fs_magic; // file->f_inode->i_sb->s_magic
        __u32 mnt_id;   // real_mount(file->f_path.mnt)->mnt_id
        __u32 nlink;    // file->f_inode->i_nlink
        __u32 mode;     // file->f_inode->i_mode
        __u32 flags;    // file->f_flags
};
and separate helper that returns struct file_handle ?
and ctx rewriting moved to fs/bpf_file_filter.c ?