Re: [PATCH v5 bpf-next 20/23] bpf: enhance BPF_JEQ/BPF_JNE is_branch_taken logic

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Oct 31, 2023 at 11:04 AM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> On Tue, Oct 31, 2023 at 9:36 AM Alexei Starovoitov
> <alexei.starovoitov@xxxxxxxxx> wrote:
> >
> > On Mon, Oct 30, 2023 at 11:16 PM Andrii Nakryiko
> > <andrii.nakryiko@xxxxxxxxx> wrote:
> > >
> > > On Mon, Oct 30, 2023 at 7:20 PM Alexei Starovoitov
> > > <alexei.starovoitov@xxxxxxxxx> wrote:
> > > >
> > > > On Fri, Oct 27, 2023 at 11:13:43AM -0700, Andrii Nakryiko wrote:
> > > > > Use 32-bit subranges to prune some 64-bit BPF_JEQ/BPF_JNE conditions
> > > > > that otherwise would be "inconclusive" (i.e., is_branch_taken() would
> > > > > return -1). This can happen, for example, when registers are initialized
> > > > > as 64-bit u64/s64, then compared for inequality as 32-bit subregisters,
> > > > > and then followed by 64-bit equality/inequality check. That 32-bit
> > > > > inequality can establish some pattern for lower 32 bits of a register
> > > > > (e.g., s< 0 condition determines whether the bit #31 is zero or not),
> > > > > while overall 64-bit value could be anything (according to a value range
> > > > > representation).
> > > > >
> > > > > This is not a fancy quirky special case, but actually a handling that's
> > > > > necessary to prevent correctness issue with BPF verifier's range
> > > > > tracking: set_range_min_max() assumes that register ranges are
> > > > > non-overlapping, and if that condition is not guaranteed by
> > > > > is_branch_taken() we can end up with invalid ranges, where min > max.
> > > >
> > > > This is_scalar_branch_taken() logic makes sense,
> > > > but if set_range_min_max() is delicate, it should have its own sanity
> > > > check for ranges.
> > > > Shouldn't be difficult to check for that dangerous overlap case.
> > >
> > > So let me clarify. As far as I'm concerned, is_branch_taken() is such
> > > a check for set_reg_min_max, and so duplicating such checks in
> > > set_reg_min_max() is just that a duplication of code and logic, and
> > > just a chance for more typos and subtle bugs.
> > >
> > > But the concern about invalid ranges is valid, so I don't know,
> > > perhaps we should just do a quick check after adjustment to validate
> > > that umin<=umax and so on? E.g., we can do that outside of
> > > reg_set_min_max(), to keep reg_set_min_max() non-failing. WDYT?
> >
> > Sounds like a good option too.
> > Just trying to minimize breakage in the future.
> > Sanity check before or after should catch it.
>
> Sounds good, I'll have a separate register state sanity check and will
> see what minimal amount of places where we should call it.
>
> I'm assuming we are ok with returning -EFAULT and failing validation
> whenever we detect violation, right?

Yep and I'll take back WARN suggestion. Let's not add any WARN to avoid
triggering panic_on_warn.





[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux