Re: [PATCH] Removed unneeded access().

[Josh Stone <jistone@redhat.com>]

On 06/13/2012 05:01 AM, Pasi Savanainen wrote:
> * ctl.c: access() is not needed because previous open() will fail
> if user has no read write accees to a command channel file.

Sorry, this is not true, because staprun is setuid-root.  The open()
should always succeed if the file exists, due to the effective UID 0,
but access() is making sure that the *real* UID (of the original user)
also has permissions for that file.

Later when the control channel is opened by stapio, your statement about
open() and access() is true, but we must be careful with staprun.

> Actually existance of access() check generates a misleading error:
> "Error, 'stap_26013' (stap_26013) is not a zombie systemtap module."
> when stap is runned by regular user (stapusr group)
> and /sys/kernel/debug is mounted as 0700.

This is true - regular users won't be able to reach anything underneath
if debugfs is mounted this way.  My first thought is, "Don't do that!"

We could at least clean up the error messages though.  I think we could
just add an access("/sys/kernel/debug", X_OK) check in staprun_funcs.c
mountfs() to make sure the user will be able to get that far.

It might also be possible to solve this more fully.  We could open the
directory in staprun and do the access check with faccessat().  Then
we'd have to also pass a control_channel fd across the exec to stapio,
since it can't open that path itself.  Given the security concerns about
all this though, I'd want stronger motivation for tinkering here.

Josh