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

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

[Rajat Jain]

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.

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);
 	}
 
 unlock:
-- 
2.32.0.432.gabb21c7263-goog

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

[Greg KH]

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

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

And what userspace tool will be looking for this?

thanks,

greg k-h

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

[Rajat Jain]

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

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

[Greg KH]

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