BPF_ALU | BPF_MOVSX with offset = 32?

BPF_ALU | BPF_MOVSX with offset = 32?

[Dave Thaler]

In re-reading the instruction-set.rst changes for sign extensions, there is one ambiguity
regarding BPF_ALU | BPF_MOVSX with offset = 32.

Is it:
a) Undefined (not a permitted instruction), or
b) Defined as being synonymous with BPF_ALU | BPF_MOV?

The table implies (b) when it says:
> BPF_MOVSX  0xb0   8/16/32  dst = (s8,s16,s32)src

But the following text could be interpreted as ():
> ``BPF_ALU | BPF_MOVSX`` :term:`sign extends<Sign Extend>` 8-bit and 16-bit operands into 32
> bit operands, and zeroes the remaining upper 32 bits.

There's no reason I can think of to use it, given it's synonymous but if given a BPF program that
uses it, should it be rejected by a verifier/disassembler/etc.?  Or treated as valid?

Dave

[Bpf] BPF_ALU | BPF_MOVSX with offset = 32?

[Dave Thaler]

In re-reading the instruction-set.rst changes for sign extensions, there is one ambiguity
regarding BPF_ALU | BPF_MOVSX with offset = 32.

Is it:
a) Undefined (not a permitted instruction), or
b) Defined as being synonymous with BPF_ALU | BPF_MOV?

The table implies (b) when it says:
> BPF_MOVSX  0xb0   8/16/32  dst = (s8,s16,s32)src

But the following text could be interpreted as ():
> ``BPF_ALU | BPF_MOVSX`` :term:`sign extends<Sign Extend>` 8-bit and 16-bit operands into 32
> bit operands, and zeroes the remaining upper 32 bits.

There's no reason I can think of to use it, given it's synonymous but if given a BPF program that
uses it, should it be rejected by a verifier/disassembler/etc.?  Or treated as valid?

Dave

-- 
Bpf mailing list
Bpf@ietf.org
https://www.ietf.org/mailman/listinfo/bpf

Re: BPF_ALU | BPF_MOVSX with offset = 32?

[Eduard Zingerman]

On Thu, 2023-09-28 at 21:35 +0000, Dave Thaler wrote:
> In re-reading the instruction-set.rst changes for sign extensions, there is one ambiguity
> regarding BPF_ALU | BPF_MOVSX with offset = 32.
> 
> Is it:
> a) Undefined (not a permitted instruction), or
> b) Defined as being synonymous with BPF_ALU | BPF_MOV?
> 
> The table implies (b) when it says:
> > BPF_MOVSX  0xb0   8/16/32  dst = (s8,s16,s32)src
> 
> But the following text could be interpreted as ():
> > ``BPF_ALU | BPF_MOVSX`` :term:`sign extends<Sign Extend>` 8-bit and 16-bit operands into 32
> > bit operands, and zeroes the remaining upper 32 bits.

Hi Dave,

I checked current verifier implementation and it goes with option (a):

    static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn)
    {
        ...
        } else if (opcode == BPF_MOV) {
            if (BPF_SRC(insn->code) == BPF_X) {
                ...
                if (BPF_CLASS(insn->code) == BPF_ALU) {
                    if (insn->off != 0 && insn->off != 8 && insn->off != 16) {
                        verbose(env, "BPF_MOV uses reserved fields\n");
                        return -EINVAL;
                    }
                } ...
                ...
            } ...
        ...
    }
    
For 32-bit move it reports error if insn->off == 32.
LLVM backend also uses option (a) as it only defines MOVSX_rr_32_8 and
MOVSX_rr_32_16, thus hypothetical MOVSX_rr_32_32 would be rejected by
disassembler.
 
> There's no reason I can think of to use it, given it's synonymous but if given a BPF program that
> uses it, should it be rejected by a verifier/disassembler/etc.?  Or treated as valid?

Atleast this is what happens now.

Thanks,
Eduard.

Re: BPF_ALU | BPF_MOVSX with offset = 32?

[Alexei Starovoitov]

On Fri, Sep 29, 2023 at 5:54 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Thu, 2023-09-28 at 21:35 +0000, Dave Thaler wrote:
> > In re-reading the instruction-set.rst changes for sign extensions, there is one ambiguity
> > regarding BPF_ALU | BPF_MOVSX with offset = 32.
> >
> > Is it:
> > a) Undefined (not a permitted instruction), or
> > b) Defined as being synonymous with BPF_ALU | BPF_MOV?
> >
> > The table implies (b) when it says:
> > > BPF_MOVSX  0xb0   8/16/32  dst = (s8,s16,s32)src
> >
> > But the following text could be interpreted as ():
> > > ``BPF_ALU | BPF_MOVSX`` :term:`sign extends<Sign Extend>` 8-bit and 16-bit operands into 32
> > > bit operands, and zeroes the remaining upper 32 bits.
>
> Hi Dave,
>
> I checked current verifier implementation and it goes with option (a):

that's correct.
I think that sentence is clear enough:
BPF_ALU | BPF_MOVSX`` :term:`sign extends<Sign Extend>` 8-bit and
16-bit operands into 32.
Which means that 24-bit, 32-bit or other bit width is not permitted.
I frankly don't see any ambiguity.

Re: [Bpf] BPF_ALU | BPF_MOVSX with offset = 32?

[Alexei Starovoitov]

On Fri, Sep 29, 2023 at 5:54 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Thu, 2023-09-28 at 21:35 +0000, Dave Thaler wrote:
> > In re-reading the instruction-set.rst changes for sign extensions, there is one ambiguity
> > regarding BPF_ALU | BPF_MOVSX with offset = 32.
> >
> > Is it:
> > a) Undefined (not a permitted instruction), or
> > b) Defined as being synonymous with BPF_ALU | BPF_MOV?
> >
> > The table implies (b) when it says:
> > > BPF_MOVSX  0xb0   8/16/32  dst = (s8,s16,s32)src
> >
> > But the following text could be interpreted as ():
> > > ``BPF_ALU | BPF_MOVSX`` :term:`sign extends<Sign Extend>` 8-bit and 16-bit operands into 32
> > > bit operands, and zeroes the remaining upper 32 bits.
>
> Hi Dave,
>
> I checked current verifier implementation and it goes with option (a):

that's correct.
I think that sentence is clear enough:
BPF_ALU | BPF_MOVSX`` :term:`sign extends<Sign Extend>` 8-bit and
16-bit operands into 32.
Which means that 24-bit, 32-bit or other bit width is not permitted.
I frankly don't see any ambiguity.

-- 
Bpf mailing list
Bpf@ietf.org
https://www.ietf.org/mailman/listinfo/bpf