diff mbox series

[v3,1/4] usb: typec: ucsi: Fix null pointer dereference in trace

Message ID 20240503003920.1482447-2-jthies@google.com (mailing list archive)
State Superseded
Headers show
Series usb: typec: ucsi: Update UCSI alternate mode | expand

Commit Message

Jameson Thies May 3, 2024, 12:39 a.m. UTC
From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>

ucsi_register_altmode checks IS_ERR on returned pointer and treats
NULL as valid. When CONFIG_TYPEC_DP_ALTMODE is not enabled
ucsi_register_displayport returns NULL which causese a NULL pointer
dereference in trace. Rather than return NULL, call
typec_port_register_altmode to register DisplayPort alternate mode
as a non-controllable mode when CONFIG_TYPEC_DP_ALTMODE is not enabled.

Reviewed-by: Jameson Thies <jthies@google.com>
Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
Changes in V3:
- Returns typec_port_register_altmode call from
ucsi_register_displayport when CONFIG_TYPEC_DP_ALTMODE is not enabled.

Changes in V2:
- Checks for error response from ucsi_register_displayport when
registering DisplayPort alternate mode.

 drivers/usb/typec/ucsi/ucsi.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Benson Leung May 3, 2024, 1:18 a.m. UTC | #1
Hi Jameson,

On Fri, May 03, 2024 at 12:39:17AM +0000, Jameson Thies wrote:
> From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> 
> ucsi_register_altmode checks IS_ERR on returned pointer and treats
> NULL as valid. When CONFIG_TYPEC_DP_ALTMODE is not enabled
> ucsi_register_displayport returns NULL which causese a NULL pointer
> dereference in trace. Rather than return NULL, call
> typec_port_register_altmode to register DisplayPort alternate mode
> as a non-controllable mode when CONFIG_TYPEC_DP_ALTMODE is not enabled.
> 
> Reviewed-by: Jameson Thies <jthies@google.com>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>

Reviewed-by: Benson Leung <bleung@chromium.org>

> ---
> Changes in V3:
> - Returns typec_port_register_altmode call from
> ucsi_register_displayport when CONFIG_TYPEC_DP_ALTMODE is not enabled.
> 
> Changes in V2:
> - Checks for error response from ucsi_register_displayport when
> registering DisplayPort alternate mode.
> 
>  drivers/usb/typec/ucsi/ucsi.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> index c4d103db9d0f8..f66224a270bc6 100644
> --- a/drivers/usb/typec/ucsi/ucsi.h
> +++ b/drivers/usb/typec/ucsi/ucsi.h
> @@ -496,7 +496,7 @@ ucsi_register_displayport(struct ucsi_connector *con,
>  			  bool override, int offset,
>  			  struct typec_altmode_desc *desc)
>  {
> -	return NULL;
> +	return typec_port_register_altmode(con->port, desc);
>  }
>  
>  static inline void
> -- 
> 2.45.0.rc1.225.g2a3ae87e7f-goog
>
Markus Elfring May 3, 2024, 3:35 p.m. UTC | #2
>                                      … which causese a NULL pointer
…

I hope that a typo will be avoided in the change description for the final commit.


…
> ---
> Changes in V3:
…

How do you think about to mention also adjustments for the commit message here?

Regards,
Markus
Jameson Thies May 3, 2024, 7:05 p.m. UTC | #3
Hi Markus,
thank you for catching the typo. If I upload a v4 patch, I will fix it
there. I don't think it is necessary to mention changes to the commit
message in the section below the commit message.

Thanks,
Jameson
Markus Elfring May 4, 2024, 5:12 a.m. UTC | #4
> I don't think it is necessary to mention changes to the commit message
> in the section below the commit message.

Did you notice that other contributors occasionally share hints about
adjustments for parts of commit messages?
Will further information presentation become better supported?

Regards,
Markus
Greg KH May 4, 2024, 7:24 a.m. UTC | #5
On Sat, May 04, 2024 at 07:12:49AM +0200, Markus Elfring wrote:
> > I don't think it is necessary to mention changes to the commit message
> > in the section below the commit message.
> 
> Did you notice that other contributors occasionally share hints about
> adjustments for parts of commit messages?
> Will further information presentation become better supported?

Hi,

This is the semi-friendly patch-bot of Greg Kroah-Hartman.

Markus, you seem to have sent a nonsensical or otherwise pointless
review comment to a patch submission on a Linux kernel developer mailing
list.  I strongly suggest that you not do this anymore.  Please do not
bother developers who are actively working to produce patches and
features with comments that, in the end, are a waste of time.

Patch submitter, please ignore Markus's suggestion; you do not need to
follow it at all.  The person/bot/AI that sent it is being ignored by
almost all Linux kernel maintainers for having a persistent pattern of
behavior of producing distracting and pointless commentary, and
inability to adapt to feedback.  Please feel free to also ignore emails
from them.

thanks,

greg k-h's patch email bot
Heikki Krogerus May 6, 2024, 10:03 a.m. UTC | #6
On Fri, May 03, 2024 at 12:39:17AM +0000, Jameson Thies wrote:
> From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> 
> ucsi_register_altmode checks IS_ERR on returned pointer and treats
> NULL as valid. When CONFIG_TYPEC_DP_ALTMODE is not enabled
> ucsi_register_displayport returns NULL which causese a NULL pointer
> dereference in trace. Rather than return NULL, call
> typec_port_register_altmode to register DisplayPort alternate mode
> as a non-controllable mode when CONFIG_TYPEC_DP_ALTMODE is not enabled.
> 
> Reviewed-by: Jameson Thies <jthies@google.com>
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>

You delivered the patch, so you should have used SoB instead of
Reviewed-by tag:
https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
> Changes in V3:
> - Returns typec_port_register_altmode call from
> ucsi_register_displayport when CONFIG_TYPEC_DP_ALTMODE is not enabled.
> 
> Changes in V2:
> - Checks for error response from ucsi_register_displayport when
> registering DisplayPort alternate mode.
> 
>  drivers/usb/typec/ucsi/ucsi.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> index c4d103db9d0f8..f66224a270bc6 100644
> --- a/drivers/usb/typec/ucsi/ucsi.h
> +++ b/drivers/usb/typec/ucsi/ucsi.h
> @@ -496,7 +496,7 @@ ucsi_register_displayport(struct ucsi_connector *con,
>  			  bool override, int offset,
>  			  struct typec_altmode_desc *desc)
>  {
> -	return NULL;
> +	return typec_port_register_altmode(con->port, desc);
>  }
>  
>  static inline void
> -- 
> 2.45.0.rc1.225.g2a3ae87e7f-goog
Greg KH May 10, 2024, 9:35 a.m. UTC | #7
On Mon, May 06, 2024 at 01:03:12PM +0300, Heikki Krogerus wrote:
> On Fri, May 03, 2024 at 12:39:17AM +0000, Jameson Thies wrote:
> > From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > 
> > ucsi_register_altmode checks IS_ERR on returned pointer and treats
> > NULL as valid. When CONFIG_TYPEC_DP_ALTMODE is not enabled
> > ucsi_register_displayport returns NULL which causese a NULL pointer
> > dereference in trace. Rather than return NULL, call
> > typec_port_register_altmode to register DisplayPort alternate mode
> > as a non-controllable mode when CONFIG_TYPEC_DP_ALTMODE is not enabled.
> > 
> > Reviewed-by: Jameson Thies <jthies@google.com>
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> 
> You delivered the patch, so you should have used SoB instead of
> Reviewed-by tag:
> https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

Not "should", that would be "must". I can't take it like this, sorry.

greg k-h
Jameson Thies May 10, 2024, 5:47 p.m. UTC | #8
Thank you for the feedback, I'll post a v4 series fixing these issues.

Thanks,
Jameson
diff mbox series

Patch

diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index c4d103db9d0f8..f66224a270bc6 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -496,7 +496,7 @@  ucsi_register_displayport(struct ucsi_connector *con,
 			  bool override, int offset,
 			  struct typec_altmode_desc *desc)
 {
-	return NULL;
+	return typec_port_register_altmode(con->port, desc);
 }
 
 static inline void