diff mbox series

[v2] usb: typec: altmodes/displayport: create sysfs nodes after assigning driver data

Message ID 20240226234228.1768725-2-rdbabiera@google.com
State New
Headers
Series [v2] usb: typec: altmodes/displayport: create sysfs nodes after assigning driver data |

Commit Message

RD Babiera Feb. 26, 2024, 11:42 p.m. UTC 
  The DisplayPort driver's sysfs nodes may be present to the userspace before
typec_altmode_set_drvdata() completes in dp_altmode_probe. This means that
a sysfs read can trigger a NULL pointer error by deferencing dp->hpd in
hpd_show or dp->lock in pin_assignment_show, as dev_get_drvdata() returns
NULL in those cases.

Create sysfs nodes after typec_altmode_set_drvdata call.

Fixes: 0e3bb7d6894d ("usb: typec: Add driver for DisplayPort alternate mode")
Cc: stable@vger.kernel.org
Signed-off-by: RD Babiera <rdbabiera@google.com>
---
Changes from v1:
* Moved sysfs node creation instead of NULL checking dev_get_drvdata().
---
 drivers/usb/typec/altmodes/displayport.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)


base-commit: a560a5672826fc1e057068bda93b3d4c98d037a2

Comments

Greg KH Feb. 27, 2024, 5:20 a.m. UTC | #1 
On Mon, Feb 26, 2024 at 11:42:29PM +0000, RD Babiera wrote:
> The DisplayPort driver's sysfs nodes may be present to the userspace before
> typec_altmode_set_drvdata() completes in dp_altmode_probe. This means that
> a sysfs read can trigger a NULL pointer error by deferencing dp->hpd in
> hpd_show or dp->lock in pin_assignment_show, as dev_get_drvdata() returns
> NULL in those cases.
> 
> Create sysfs nodes after typec_altmode_set_drvdata call.
> 
> Fixes: 0e3bb7d6894d ("usb: typec: Add driver for DisplayPort alternate mode")
> Cc: stable@vger.kernel.org
> Signed-off-by: RD Babiera <rdbabiera@google.com>
> ---
> Changes from v1:
> * Moved sysfs node creation instead of NULL checking dev_get_drvdata().
> ---
>  drivers/usb/typec/altmodes/displayport.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
> index 5a80776c7255..5bbdd2c04237 100644
> --- a/drivers/usb/typec/altmodes/displayport.c
> +++ b/drivers/usb/typec/altmodes/displayport.c
> @@ -731,10 +731,6 @@ int dp_altmode_probe(struct typec_altmode *alt)
>  	      DP_CAP_PIN_ASSIGN_DFP_D(alt->vdo)))
>  		return -ENODEV;
>  
> -	ret = sysfs_create_group(&alt->dev.kobj, &dp_altmode_group);
> -	if (ret)
> -		return ret;
> -
>  	dp = devm_kzalloc(&alt->dev, sizeof(*dp), GFP_KERNEL);
>  	if (!dp)
>  		return -ENOMEM;
> @@ -766,6 +762,10 @@ int dp_altmode_probe(struct typec_altmode *alt)
>  	if (plug)
>  		typec_altmode_set_drvdata(plug, dp);
>  
> +	ret = sysfs_create_group(&alt->dev.kobj, &dp_altmode_group);
> +	if (ret)
> +		return ret;
> +

Now I am going to push back again and ask why you are even attempting to
create sysfs files "by hand" here at all?

Why is this just not set to be a default group?  That way the group is
managed properly by the driver core and the driver doesn't have to worry
about ANY of this at all.  Bonus is that you remove the "you raced with
userspace and lost" problem that this code still has even with the
change you made here.

Huge hint, if you EVER have to call sysfs_*() from a driver, usually
something is wrong (there are a few exceptions, this is not one.)

thanks,

greg k-h
RD Babiera Feb. 28, 2024, 10:59 p.m. UTC | #2 
On Mon, Feb 26, 2024 at 9:20 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> Now I am going to push back again and ask why you are even attempting to
> create sysfs files "by hand" here at all?
>
> Why is this just not set to be a default group?  That way the group is
> managed properly by the driver core and the driver doesn't have to worry
> about ANY of this at all.  Bonus is that you remove the "you raced with
> userspace and lost" problem that this code still has even with the
> change you made here.

To answer both questions, the driver had always created its sysfs
nodes manually so I did not suspect this was not the preferred way
to handle driver sysfs creation until I found your article on default attributes
after your email. Will fix the patch up.

thanks again,
rd
diff mbox series

Patch

diff --git a/drivers/usb/typec/altmodes/displayport.c b/drivers/usb/typec/altmodes/displayport.c
index 5a80776c7255..5bbdd2c04237 100644
--- a/drivers/usb/typec/altmodes/displayport.c
+++ b/drivers/usb/typec/altmodes/displayport.c
@@ -731,10 +731,6 @@  int dp_altmode_probe(struct typec_altmode *alt)
 	      DP_CAP_PIN_ASSIGN_DFP_D(alt->vdo)))
 		return -ENODEV;
 
-	ret = sysfs_create_group(&alt->dev.kobj, &dp_altmode_group);
-	if (ret)
-		return ret;
-
 	dp = devm_kzalloc(&alt->dev, sizeof(*dp), GFP_KERNEL);
 	if (!dp)
 		return -ENOMEM;
@@ -766,6 +762,10 @@  int dp_altmode_probe(struct typec_altmode *alt)
 	if (plug)
 		typec_altmode_set_drvdata(plug, dp);
 
+	ret = sysfs_create_group(&alt->dev.kobj, &dp_altmode_group);
+	if (ret)
+		return ret;
+
 	dp->state = plug ? DP_STATE_ENTER_PRIME : DP_STATE_ENTER;
 	schedule_work(&dp->work);