Re: [PATCH bpf V2 1/1] bpf: fix verification of indirect var-off stack access

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

 



On Thu, Dec 7, 2023 at 7:23 PM Andrei Matei <andreimatei1@xxxxxxxxx> wrote:
>
> [...]
>
> > >
> > > Ack. Still, if you don't mind entertaining me further, two more questions:
> > >
> > > 1. What do you make of the code in check_mem_size_reg() [1] where we do
> > >
> > > if (reg->umin_value == 0) {
> > >   err = check_helper_mem_access(env, regno - 1, 0,
> > >         zero_size_allowed,
> > >         meta);
> > >
> > > followed by
> > >
> > > err = check_helper_mem_access(env, regno - 1,
> > >       reg->umax_value,
> > >       zero_size_allowed, meta);
> > >
> > > [1] https://github.com/torvalds/linux/blob/bee0e7762ad2c6025b9f5245c040fcc36ef2bde8/kernel/bpf/verifier.c#L7486-L7489
> > >
> > > What's the point of the first check_helper_mem_access() call - the
> > > zero-sized one
> > > (given that we also have the second, broader, check)? Could it be
> > > simply replaced by a
> > >
> > > if (reg->umin_value == 0 && !zero_sized_allowed)
> > >     err = no_bueno;
> > >
> >
> > Maybe Kumar (cc'ed) can chime in as well, but I suspect that's exactly
> > this, and kind of similar to the min_off/max_off discussion we had. So
> > yes, I suspect the above simple and straightforward check would be
> > much more meaningful and targeted.
>
> I plan on trying this in a bit; sounds like you're encouraging it.
>
> >
> > I gotta say that the reg->smin_value < 0 check is confusing, though,
> > I'm not sure why we are mixing smin and umin/umax in this change...
>
> When you say "in this change", you mean in the existing code, yeah?  I'm not

Yeah, sorry, words are hard. It's clearly a question about pre-existing code.

> familiar enough with the smin/umin tracking to tell if `reg->smin_value >= 0`
> (the condition that the function tests first) implies that
> `reg->smin_value == reg->umin_value` (i.e. the fact that we're currently mixing

this is probably true most of the time, but knowing how tricky this
range tracking is, there is non-zero chance that this is not always
true. Which is why I'm a bit confused why we are freely intermixing
signed/unsigned range in this code.

> smin/umin in check_mem_size_reg() is confusing, but benign).  Is that true? If
> so, are you saying that check_mem_size_reg() should exclusively use smin/smax?
>

I'd like to hear from Kumar on what was the intent before suggesting
changing anything.

>
> [...]





[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