On Thu, Feb 13, 2025 at 1:16 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote: > > On Wed, Feb 12, 2025 at 05:37:11PM -0800, Andy Lutomirski wrote: > > On Wed, Feb 12, 2025 at 2:04 PM Jiri Olsa <jolsa@xxxxxxxxxx> wrote: > > > > > > Jann reported [1] possible issue when trampoline_check_ip returns > > > address near the bottom of the address space that is allowed to > > > call into the syscall if uretprobes are not set up. > > > > > > Though the mmap minimum address restrictions will typically prevent > > > creating mappings there, let's make sure uretprobe syscall checks > > > for that. > > > > It would be a layering violation, but we could perhaps do better here: > > > > > - if (regs->ip != trampoline_check_ip()) > > > + /* Make sure the ip matches the only allowed sys_uretprobe caller. */ > > > + if (unlikely(regs->ip != trampoline_check_ip(tramp))) > > > goto sigill; > > > > Instead of SIGILL, perhaps this should do the seccomp action? So the > > logic in seccomp would be (sketchily, with some real mode1 mess): > > > > if (is_a_real_uretprobe()) > > skip seccomp; > > IIUC you want to move the address check earlier to the seccomp path.. > with the benefit that we would kill not allowed caller sooner? The benefit would be that seccomp users that want to do something other than killing a process (returning an error code, getting notified, etc) could retain that functionality without the new automatic hole being poked for uretprobe() in cases where uprobes aren't in use or where the calling address doesn't match the uprobe trampoline. IOW it would reduce the scope to which we're making seccomp behave unexpectedly. > > jirka > > > > > where is_a_real_uretprobe() is only true if the nr and arch match > > uretprobe *and* the address is right. > > > > --Andy >
