On Fri, Jul 7, 2023 at 2:08 PM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Fri, Jul 7, 2023 at 9:44 AM Alexei Starovoitov > <alexei.starovoitov@xxxxxxxxx> wrote: > > > > On Fri, Jul 7, 2023 at 7:04 AM Andrew Werner <awerner32@xxxxxxxxx> wrote: > > > > > > When it comes to fixing the problem, I don't quite know where to start. > > > Perhaps these iteration callbacks ought to be treated more like global functions > > > -- you can't always make assumptions about the state of the data in the context > > > pointer. Treating the context pointer as totally opaque seems bad from > > > a usability > > > perspective. Maybe there's a way to attempt to verify the function > > > body of the function > > > by treating all or part of the context as read-only, and then if that > > > fails, go back and > > > assume nothing about that part of the context structure. What is the > > > right way to > > > think about plugging this hole? > > > > 'treat as global' might be a way to fix it, but it will likely break > > some setups, since people passing pointers in a context and current > > global func verification doesn't support that. > > yeah, the intended use case is to return something from callbacks > through context pointer. So it will definitely break real uses. Definitely, this would break the probes we're using. > > > I think the simplest fix would be to disallow writes into any pointers > > within a ctx. Writes to scalars should still be allowed. > > It might be a partial mitigation, but even with SCALARs there could be > problems due to assumed SCALAR range, which will be invalid if > callback is never called or called many times. Indeed when the bug was first found it was because an offset scalar in the context was being modified and was being added to an unmodified pointer. > > Much more complex fix would be to verify callbacks as > > process_iter_next_call(). See giant comment next to it. > > yep, that seems like the right way forward. We need to simulate 0, 1, > 2, .... executions of callbacks until we validate and exhaust all > possible context modification permutations, just like open-coded > iterators do it I'll give process_iter_next_call() a shot, and see if I run into trouble. On Fri, Jul 7, 2023 at 2:08 PM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > On Fri, Jul 7, 2023 at 9:44 AM Alexei Starovoitov > <alexei.starovoitov@xxxxxxxxx> wrote: > > > > On Fri, Jul 7, 2023 at 7:04 AM Andrew Werner <awerner32@xxxxxxxxx> wrote: > > > > > > When it comes to fixing the problem, I don't quite know where to start. > > > Perhaps these iteration callbacks ought to be treated more like global functions > > > -- you can't always make assumptions about the state of the data in the context > > > pointer. Treating the context pointer as totally opaque seems bad from > > > a usability > > > perspective. Maybe there's a way to attempt to verify the function > > > body of the function > > > by treating all or part of the context as read-only, and then if that > > > fails, go back and > > > assume nothing about that part of the context structure. What is the > > > right way to > > > think about plugging this hole? > > > > 'treat as global' might be a way to fix it, but it will likely break > > some setups, since people passing pointers in a context and current > > global func verification doesn't support that. > > yeah, the intended use case is to return something from callbacks > through context pointer. So it will definitely break real uses. > > > I think the simplest fix would be to disallow writes into any pointers > > within a ctx. Writes to scalars should still be allowed. > > It might be a partial mitigation, but even with SCALARs there could be > problems due to assumed SCALAR range, which will be invalid if > callback is never called or called many times. > > > Much more complex fix would be to verify callbacks as > > process_iter_next_call(). See giant comment next to it. > > yep, that seems like the right way forward. We need to simulate 0, 1, > 2, .... executions of callbacks until we validate and exhaust all > possible context modification permutations, just like open-coded > iterators do it > > > But since we need a fix for stable I'd try the simple approach first. > > Could you try to implement that?
