| 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
| ||
|
Date: Fri, 13 Mar 2015 19:07:15 -0700
From: Alexei Starovoitov <ast@...mgrid.com>
To: Daniel Borkmann <daniel@...earbox.net>,
"David S. Miller" <davem@...emloft.net>
CC: Thomas Graf <tgraf@...g.ch>, linux-api@...r.kernel.org,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 net-next 1/2] bpf: allow extended BPF programs access
skb fields
On 3/13/15 6:46 PM, Daniel Borkmann wrote:
> On 03/13/2015 07:57 PM, Alexei Starovoitov wrote:
>> introduce user accessible mirror of in-kernel 'struct sk_buff':
>> struct __sk_buff {
>> __u32 len;
>> __u32 pkt_type;
>> __u32 mark;
>> __u32 queue_mapping;
>> };
>>
>> bpf programs can do:
>>
>> int bpf_prog(struct __sk_buff *skb)
>> {
>> __u32 var = skb->pkt_type;
>>
>> which will be compiled to bpf assembler as:
>>
>> dst_reg = *(u32 *)(src_reg + 4) // 4 == offsetof(struct __sk_buff,
>> pkt_type)
>>
>> bpf verifier will check validity of access and will convert it to:
>>
>> dst_reg = *(u8 *)(src_reg + offsetof(struct sk_buff, __pkt_type_offset))
>> dst_reg &= 7
>>
>> since skb->pkt_type is a bitfield.
>>
>> Signed-off-by: Alexei Starovoitov <ast@...mgrid.com>
> ...
>> +static u32 convert_skb_access(int skb_field, int dst_reg, int src_reg,
>> + struct bpf_insn *insn_buf)
>> +{
>> + struct bpf_insn *insn = insn_buf;
>> +
>> + switch (skb_field) {
>> + case SKF_AD_MARK:
>> + BUILD_BUG_ON(FIELD_SIZEOF(struct sk_buff, mark) != 4);
>> +
>> + *insn++ = BPF_LDX_MEM(BPF_W, dst_reg, src_reg,
>> + offsetof(struct sk_buff, mark));
>> + break;
>> +
>> + case SKF_AD_PKTTYPE:
>> + *insn++ = BPF_LDX_MEM(BPF_B, dst_reg, src_reg,
>> PKT_TYPE_OFFSET());
>> + *insn++ = BPF_ALU32_IMM(BPF_AND, dst_reg, PKT_TYPE_MAX);
>> +#ifdef __BIG_ENDIAN_BITFIELD
>> + *insn++ = BPF_ALU32_IMM(BPF_RSH, dst_reg, 5);
>> +#endif
>> + break;
>> +
>> + case SKF_AD_QUEUE:
>> + BUILD_BUG_ON(FIELD_SIZEOF(struct sk_buff, queue_mapping) != 2);
>> +
>> + *insn++ = BPF_LDX_MEM(BPF_H, dst_reg, src_reg,
>> + offsetof(struct sk_buff, queue_mapping));
>> + break;
>> + }
>> +
>> + return insn - insn_buf;
>> +}
>> +
>> static bool convert_bpf_extensions(struct sock_filter *fp,
>> struct bpf_insn **insnp)
>> {
>> struct bpf_insn *insn = *insnp;
>> + u32 cnt;
>>
>> switch (fp->k) {
>> case SKF_AD_OFF + SKF_AD_PROTOCOL:
>> @@ -167,13 +200,8 @@ static bool convert_bpf_extensions(struct
>> sock_filter *fp,
>> break;
>>
>> case SKF_AD_OFF + SKF_AD_PKTTYPE:
>> - *insn++ = BPF_LDX_MEM(BPF_B, BPF_REG_A, BPF_REG_CTX,
>> - PKT_TYPE_OFFSET());
>> - *insn = BPF_ALU32_IMM(BPF_AND, BPF_REG_A, PKT_TYPE_MAX);
>> -#ifdef __BIG_ENDIAN_BITFIELD
>> - insn++;
>> - *insn = BPF_ALU32_IMM(BPF_RSH, BPF_REG_A, 5);
>> -#endif
>> + cnt = convert_skb_access(SKF_AD_PKTTYPE, BPF_REG_A,
>> BPF_REG_CTX, insn);
>> + insn += cnt - 1;
>> break;
>>
>> case SKF_AD_OFF + SKF_AD_IFINDEX:
>> @@ -197,10 +225,8 @@ static bool convert_bpf_extensions(struct
>> sock_filter *fp,
>> break;
>>
>> case SKF_AD_OFF + SKF_AD_MARK:
>> - BUILD_BUG_ON(FIELD_SIZEOF(struct sk_buff, mark) != 4);
>> -
>> - *insn = BPF_LDX_MEM(BPF_W, BPF_REG_A, BPF_REG_CTX,
>> - offsetof(struct sk_buff, mark));
>> + cnt = convert_skb_access(SKF_AD_MARK, BPF_REG_A, BPF_REG_CTX,
>> insn);
>> + insn += cnt - 1;
>> break;
>>
>> case SKF_AD_OFF + SKF_AD_RXHASH:
>> @@ -211,10 +237,8 @@ static bool convert_bpf_extensions(struct
>> sock_filter *fp,
>> break;
>>
>> case SKF_AD_OFF + SKF_AD_QUEUE:
>> - BUILD_BUG_ON(FIELD_SIZEOF(struct sk_buff, queue_mapping) != 2);
>> -
>> - *insn = BPF_LDX_MEM(BPF_H, BPF_REG_A, BPF_REG_CTX,
>> - offsetof(struct sk_buff, queue_mapping));
>> + cnt = convert_skb_access(SKF_AD_QUEUE, BPF_REG_A,
>> BPF_REG_CTX, insn);
>> + insn += cnt - 1;
>> break;
>>
>> case SKF_AD_OFF + SKF_AD_VLAN_TAG:
>> @@ -1147,13 +1171,55 @@ sk_filter_func_proto(enum bpf_func_id func_id)
> ...
>> +static u32 sk_filter_convert_ctx_access(int dst_reg, int src_reg, int
>> ctx_off,
>> + struct bpf_insn *insn_buf)
>> +{
>> + struct bpf_insn *insn = insn_buf;
>> +
>> + switch (ctx_off) {
>> + case offsetof(struct __sk_buff, len):
>> + BUILD_BUG_ON(FIELD_SIZEOF(struct sk_buff, len) != 4);
>> +
>> + *insn++ = BPF_LDX_MEM(BPF_W, dst_reg, src_reg,
>> + offsetof(struct sk_buff, len));
>> + break;
>> +
>> + case offsetof(struct __sk_buff, mark):
>> + return convert_skb_access(SKF_AD_MARK, dst_reg, src_reg, insn);
>> +
>> + case offsetof(struct __sk_buff, pkt_type):
>> + return convert_skb_access(SKF_AD_PKTTYPE, dst_reg, src_reg,
>> insn);
>> +
>> + case offsetof(struct __sk_buff, queue_mapping):
>> + return convert_skb_access(SKF_AD_QUEUE, dst_reg, src_reg, insn);
>> + }
>> +
>> + return insn - insn_buf;
>> }
>
> Hmm, I actually liked the previous version much better. :(
>
> Now, some members use convert_skb_access() and some skb members are
> converted directly in-place in both, convert_bpf_extensions() _and_
> in sk_filter_convert_ctx_access().
>
> Previously, it was much more consistent, which I like better. And only
> because of the simple BUILD_BUG_ON()? :/
not because of single build_bug_on, but because of having
a single place to adjust offsets and sizes when location of
sk_buff fields changes. that's the main advantage and it's a big one.
imo it's much cleaner than previous approach.
--
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