Re: [PATCH] thunderbolt: For dev authorization changes, include the actual event in udev change notification

[Greg KH <gregkh@linuxfoundation.org>]

On Sun, Jul 25, 2021 at 11:46:32AM -0700, Rajat Jain wrote:
> Sorry, Had hit "Reply"" while responding, instead of "Reply All" - so
> it went only to Greg. Now added back everyone else.
> 
> 
> On Fri, Jul 23, 2021 at 4:43 PM Rajat Jain <rajatja@google.com> wrote:
> >
> > Hello,
> >
> >
> > On Fri, Jul 23, 2021 at 12:05 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Thu, Jul 22, 2021 at 06:28:34PM -0700, Rajat Jain wrote:
> > > > For security, we would like to monitor and track when the
> > > > thunderbolt devices are authorized and deauthorized. Currently
> > > > the userspace gets a udev change notification when there is a
> > > > change, but the state may have changed (again) by the time we
> > > > look at the authorized attribute in sysfs. So an authorization
> > > > event may go unnoticed. Thus make it easier by informing the
> > > > actual change (authorized/deauthorized) in the udev change
> > > > notification.
> > >
> > > We do have 72 columns to work with... :)
> >
> > Sorry, fixed now.
> >
> > >
> > > >
> > > > Signed-off-by: Rajat Jain <rajatja@google.com>
> > > > ---
> > > >  drivers/thunderbolt/switch.c | 8 ++++++--
> > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
> > > > index 83b1ef3d5d03..5d3e9dcba44a 100644
> > > > --- a/drivers/thunderbolt/switch.c
> > > > +++ b/drivers/thunderbolt/switch.c
> > > > @@ -1499,6 +1499,7 @@ static ssize_t authorized_show(struct device *dev,
> > > >  static int disapprove_switch(struct device *dev, void *not_used)
> > > >  {
> > > >       struct tb_switch *sw;
> > > > +     char *envp[] = { "AUTHORIZED=0", NULL };
> > > >
> > > >       sw = tb_to_switch(dev);
> > > >       if (sw && sw->authorized) {
> > > > @@ -1514,7 +1515,7 @@ static int disapprove_switch(struct device *dev, void *not_used)
> > > >                       return ret;
> > > >
> > > >               sw->authorized = 0;
> > > > -             kobject_uevent(&sw->dev.kobj, KOBJ_CHANGE);
> > > > +             kobject_uevent_env(&sw->dev.kobj, KOBJ_CHANGE, envp);
> > > >       }
> > > >
> > > >       return 0;
> > > > @@ -1523,6 +1524,8 @@ static int disapprove_switch(struct device *dev, void *not_used)
> > > >  static int tb_switch_set_authorized(struct tb_switch *sw, unsigned int val)
> > > >  {
> > > >       int ret = -EINVAL;
> > > > +     char envp_string[13];
> > > > +     char *envp[] = { envp_string, NULL };
> > > >
> > > >       if (!mutex_trylock(&sw->tb->lock))
> > > >               return restart_syscall();
> > > > @@ -1560,7 +1563,8 @@ static int tb_switch_set_authorized(struct tb_switch *sw, unsigned int val)
> > > >       if (!ret) {
> > > >               sw->authorized = val;
> > > >               /* Notify status change to the userspace */
> > > > -             kobject_uevent(&sw->dev.kobj, KOBJ_CHANGE);
> > > > +             sprintf(envp_string, "AUTHORIZED=%u", val);
> > > > +             kobject_uevent_env(&sw->dev.kobj, KOBJ_CHANGE, envp);
> > >
> > > So now "val" is a userspace visable value?  Is that documented anywhere
> > > what it is and what are you going to do to ensure it never changes in
> > > the future?
> > >
> > > Also this new value "field" should be documented somewhere as well,
> > > otherwise how will any tool know it is there?
> >
> > Sorry I should have clarified and elaborated (now done in the new
> > commit log). The field / value being exposed is that of the existing
> > sysfs attribute "authorized"
> > (/sys/bus/thunderbolt/devices/.../authorized), which is already
> > documented. I made it clearer in the commit log now. I looked at other
> > uses of kobject_uevent_env() and couldn't find examples of documenting
> > the Udev environment in Documentation/.

Perhaps a comment here showing that this is the same value as that
specific sysfs attribute as well?

> > > And what userspace tool will be looking for this?
> >
> > It will likely be a udev rule which will trigger a script when it see
> > device authorization change event. Something like this:
> > SUBSYSTEM=="thunderbolt", ACTION=="change", ENV{AUTHORIZED}=="1",
> > RUN+="alert.sh"
> >
> > However, now that I say it, is it possible to check for such (kernel
> > supplied) udev event environment key value pair, using
> > udev_device_get_property_value()? If so, that makes it very easy for
> > us, and the tool to use it would be Chromeos daemon called
> > cros_healthd.

It's been a long time since I last touched the udev codebase, sorry, try
it out yourself and see!

thanks,

greg k-h