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@xxxxxxxxxx> wrote:
>
> Hello,
>
>
> On Fri, Jul 23, 2021 at 12:05 AM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> > > ---
> > > 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/.
>
> >
> > 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.
>
> Thanks,
>
> Rajat
>
> >
> > thanks,
> >
> > greg k-h
