Re: [PATCHv3 perf/core] uprobes: Harden uretprobe syscall trampoline check

* Jiri Olsa [off-list ref] wrote:

On Mon, Feb 17, 2025 at 01:09:43PM +0100, Jiri Olsa wrote:

quoted

On Thu, Feb 13, 2025 at 09:58:29AM -0800, Andy Lutomirski wrote:

quoted

On Thu, Feb 13, 2025 at 1:16 AM Jiri Olsa [off-list ref] wrote:

quoted

On Wed, Feb 12, 2025 at 05:37:11PM -0800, Andy Lutomirski wrote:

quoted

On Wed, Feb 12, 2025 at 2:04 PM Jiri Olsa [off-list ref] wrote:

quoted

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:

quoted

-       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.

Kees, any thoughts about this approach?

ping, any idea?

So in any case I think the seccomp QoL tie-in suggested by Andy should 
be done in a separate patch, and I've applied the -v3 patch to 
tip:perf/core as-is.

( I've added Alexei's Acked-by too, which as I've read the v2 thread's 
  discussion was a given as long as his ~0 suggestion was implemented,
  which you did. )

Thanks,

	Ingo