lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Tue, 27 Feb 2024 06:20:06 +0100
From: Greg KH <gregkh@...uxfoundation.org>
To: RD Babiera <rdbabiera@...gle.com>
Cc: heikki.krogerus@...ux.intel.com, badhri@...gle.com,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
	stable@...r.kernel.org
Subject: Re: [PATCH v2] usb: typec: altmodes/displayport: create sysfs nodes
 after assigning driver data

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@...r.kernel.org
> Signed-off-by: RD Babiera <rdbabiera@...gle.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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ