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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 16 Sep 2014 13:44:12 -0700 From: Alexei Starovoitov <ast@...mgrid.com> To: David Miller <davem@...emloft.net> Cc: Ingo Molnar <mingo@...nel.org>, Linus Torvalds <torvalds@...ux-foundation.org>, Andy Lutomirski <luto@...capital.net>, Daniel Borkmann <dborkman@...hat.com>, Hannes Frederic Sowa <hannes@...essinduktion.org>, Chema Gonzalez <chema@...gle.com>, Eric Dumazet <edumazet@...gle.com>, Peter Zijlstra <a.p.zijlstra@...llo.nl>, Pablo Neira Ayuso <pablo@...filter.org>, "H. Peter Anvin" <hpa@...or.com>, Andrew Morton <akpm@...ux-foundation.org>, Kees Cook <keescook@...omium.org>, Linux API <linux-api@...r.kernel.org>, Network Development <netdev@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org> Subject: Re: [PATCH v12 net-next 03/11] bpf: add lookup/update/delete/iterate methods to BPF maps On Tue, Sep 16, 2014 at 12:16 PM, David Miller <davem@...emloft.net> wrote: > From: Alexei Starovoitov <ast@...mgrid.com> > Date: Mon, 15 Sep 2014 12:18:33 -0700 > >> @@ -83,6 +112,15 @@ union bpf_attr { >> __u32 value_size; /* size of value in bytes */ >> __u32 max_entries; /* max number of entries in a map */ >> }; >> + >> + struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */ >> + int map_fd; >> + void __user *key; >> + union { >> + void __user *value; >> + void __user *next_key; >> + }; >> + }; >> }; >> >> #endif /* _UAPI__LINUX_BPF_H__ */ > > Depending upon the processor ABI, this change can increase the > alignment requirements of union bpf_attr. So the structure is not > compatible between patch #1 and patch #3 here. the union indeed changes alignment from patch #1 to #3, but, imo, it is not a problem, since kernel does: /* copy attributes from user space, may be less than sizeof(bpf_attr) */ if (copy_from_user(attr, uattr, size) != 0) and then proceeds with further bpf_attr validation, so even if user space alignment is 4 and user assumes contents from patch 1, but kernel alignment is 8 and contents from patch 3, it is still ok. Backwards compatibility is preserved. > Also, you haven't implemented any compat layer whatsoever for the > necessary translations. This happens because you are using pointers > which are different sized between 32-bit and 64-bit ABIs. I mentioned it in the cover letter: - implemented and tested compat support (not part of this set) I'll roll compat layer into the set to make it less confusing. > I would suggest you use instead something like "aligned_u64" since > these are just arbitrary userland cookies and using "aligned_u64" > vs. "u64" will make it so that you don't have to deal with the 64-bit > type alignment differences between x86-32 and x86-64 while writing the > compat wrappers (if any). I haven't thought of 'aligned_u64' for this case. For counters and masks it would be perfect, but here user is passing real pointers to key and value, so they have to be 'void __user *', otherwise user would need to type cast them, which I want to avoid. >> + struct { /* anonymous struct used by BPF_PROG_LOAD command */ >> + __u32 prog_type; /* one of enum bpf_prog_type */ >> + __u32 insn_cnt; >> + const struct bpf_insn __user *insns; >> + const char __user *license; >> + }; >> +}; > > Again, these need syscall compat handling. You may want to redesign > the types so that perhaps no compat layer translations will be needed. 'foo __user *' pointers vs 'aligned_u64'... It's a trade off. I can make all pointer fields to be u64 and then, indeed, no compat layer will be needed, but user would need to do ugly type casts. I think compat layer is a better option. It's simple enough. I'll roll it into the set and respin. Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists