| Message ID | 20240424014821.4154159-2-jthies@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: ucsi: Update UCSI alternate mode | expand |
On Wed, 24 Apr 2024 at 04:48, Jameson Thies <jthies@google.com> wrote: > > From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > ucsi_register_altmode checks IS_ERR on returned pointer and treats > NULL as valid. This results in a null deref when > trace_ucsi_register_altmode is called. Return an error from > ucsi_register_displayport when it is not supported and register the > altmode with typec_port_register_altmode. > > Reviewed-by: Jameson Thies <jthies@google.com> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > --- > Changes in V2: > - Checks for error response from ucsi_register_displayport when > registering DisplayPort alternate mode. > > drivers/usb/typec/ucsi/ucsi.c | 3 +++ > drivers/usb/typec/ucsi/ucsi.h | 2 +- > 2 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c > index cb52e7b0a2c5c..f3b413f94fd28 100644 > --- a/drivers/usb/typec/ucsi/ucsi.c > +++ b/drivers/usb/typec/ucsi/ucsi.c > @@ -361,6 +361,9 @@ static int ucsi_register_altmode(struct ucsi_connector *con, > switch (desc->svid) { > case USB_TYPEC_DP_SID: > alt = ucsi_register_displayport(con, override, i, desc); > + if (IS_ERR(alt) && PTR_ERR(alt) == -EOPNOTSUPP) This makes it ignore EOPNOTSUPP if it is returned by the non-stub implementation. I think the current state is actually better than the implementation found in this patch. I'd suggest adding a comment to ucsi_register_displayport() stub instead. > + alt = typec_port_register_altmode(con->port, desc); > + > break; > case USB_TYPEC_NVIDIA_VLINK_SID: > if (desc->vdo == USB_TYPEC_NVIDIA_VLINK_DBG_VDO) > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h > index c4d103db9d0f8..c663dce0659ee 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 ERR_PTR(-EOPNOTSUPP); > } > > static inline void > -- > 2.44.0.769.g3c40516874-goog >
On Tue, Apr 23, 2024 at 7:06 PM Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > On Wed, 24 Apr 2024 at 04:48, Jameson Thies <jthies@google.com> wrote: > > > > From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > > > ucsi_register_altmode checks IS_ERR on returned pointer and treats > > NULL as valid. This results in a null deref when > > trace_ucsi_register_altmode is called. Return an error from > > ucsi_register_displayport when it is not supported and register the > > altmode with typec_port_register_altmode. > > > > Reviewed-by: Jameson Thies <jthies@google.com> > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > --- > > Changes in V2: > > - Checks for error response from ucsi_register_displayport when > > registering DisplayPort alternate mode. > > > > drivers/usb/typec/ucsi/ucsi.c | 3 +++ > > drivers/usb/typec/ucsi/ucsi.h | 2 +- > > 2 files changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c > > index cb52e7b0a2c5c..f3b413f94fd28 100644 > > --- a/drivers/usb/typec/ucsi/ucsi.c > > +++ b/drivers/usb/typec/ucsi/ucsi.c > > @@ -361,6 +361,9 @@ static int ucsi_register_altmode(struct ucsi_connector *con, > > switch (desc->svid) { > > case USB_TYPEC_DP_SID: > > alt = ucsi_register_displayport(con, override, i, desc); > > + if (IS_ERR(alt) && PTR_ERR(alt) == -EOPNOTSUPP) > > This makes it ignore EOPNOTSUPP if it is returned by the non-stub > implementation. I think the current state is actually better than the > implementation found in this patch. I'd suggest adding a comment to > ucsi_register_displayport() stub instead. So originally on my system, I didn't have the displayport driver config enabled. My expectation was that the alt-mode would show up but would not be controllable (like all other alt-modes without drivers). What ends up happening is that no alt-mode shows up and trying to enable the trace crashes. When the displayport support isn't there, I think it should just be enumerated as a normal, unsupported alt-mode. > > > + alt = typec_port_register_altmode(con->port, desc); > > + > > break; > > case USB_TYPEC_NVIDIA_VLINK_SID: > > if (desc->vdo == USB_TYPEC_NVIDIA_VLINK_DBG_VDO) > > diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h > > index c4d103db9d0f8..c663dce0659ee 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 ERR_PTR(-EOPNOTSUPP); > > } > > > > static inline void > > -- > > 2.44.0.769.g3c40516874-goog > > > > > -- > With best wishes > Dmitry
Hi, On Wed, Apr 24, 2024 at 05:00:24PM -0700, Abhishek Pandit-Subedi wrote: > On Tue, Apr 23, 2024 at 7:06 PM Dmitry Baryshkov > <dmitry.baryshkov@linaro.org> wrote: > > > > On Wed, 24 Apr 2024 at 04:48, Jameson Thies <jthies@google.com> wrote: > > > > > > From: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > > > > > ucsi_register_altmode checks IS_ERR on returned pointer and treats > > > NULL as valid. This results in a null deref when > > > trace_ucsi_register_altmode is called. Return an error from > > > ucsi_register_displayport when it is not supported and register the > > > altmode with typec_port_register_altmode. > > > > > > Reviewed-by: Jameson Thies <jthies@google.com> > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> > > > --- > > > Changes in V2: > > > - Checks for error response from ucsi_register_displayport when > > > registering DisplayPort alternate mode. > > > > > > drivers/usb/typec/ucsi/ucsi.c | 3 +++ > > > drivers/usb/typec/ucsi/ucsi.h | 2 +- > > > 2 files changed, 4 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c > > > index cb52e7b0a2c5c..f3b413f94fd28 100644 > > > --- a/drivers/usb/typec/ucsi/ucsi.c > > > +++ b/drivers/usb/typec/ucsi/ucsi.c > > > @@ -361,6 +361,9 @@ static int ucsi_register_altmode(struct ucsi_connector *con, > > > switch (desc->svid) { > > > case USB_TYPEC_DP_SID: > > > alt = ucsi_register_displayport(con, override, i, desc); > > > + if (IS_ERR(alt) && PTR_ERR(alt) == -EOPNOTSUPP) > > > > This makes it ignore EOPNOTSUPP if it is returned by the non-stub > > implementation. I think the current state is actually better than the > > implementation found in this patch. I'd suggest adding a comment to > > ucsi_register_displayport() stub instead. > > So originally on my system, I didn't have the displayport driver > config enabled. My expectation was that the alt-mode would show up but > would not be controllable (like all other alt-modes without drivers). > What ends up happening is that no alt-mode shows up and trying to > enable the trace crashes. > > When the displayport support isn't there, I think it should just be > enumerated as a normal, unsupported alt-mode. This sounds reasonable. Thus if displayport support is not compiled in ucsi_register_displayport() should just call typec_port_register_altmode(), I guess. Best regards, Christian
… > ucsi_register_altmode checks IS_ERR on returned pointer and treats > NULL as valid. This results in a null deref when > trace_ucsi_register_altmode is called. … Can it be nicer to use the term “null pointer dereference” for the commit message here? Regards, Markus
On Thu, Apr 25, 2024 at 10:51:53AM +0200, Markus Elfring wrote: > … > > ucsi_register_altmode checks IS_ERR on returned pointer and treats > > NULL as valid. This results in a null deref when > > trace_ucsi_register_altmode is called. > … > > Can it be nicer to use the term “null pointer dereference” for > the commit message here? > > Regards, > Markus 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
Hi Dmitry, what are your thoughts on Abhishek's comment? I think we should attempt to register the alternate mode when CONFIG_TYPEC_DP_ALTMODE is not enabled. It would give us a more accurate representation of the partner in user space. I understand your point about ignoring a potential EOPNOTSUPP response from the non-stub function. What if we leave ucsi.c alone, and replace the stub function's null return with a call to typec_port_register_altmode(). That would register DP altmode as an unsupported mode when CONFIG_TYPEC_DP_ALTMODE is not enabled, and fix the null pointer dereference. But, it won't change behavior when CONFIG_TYPEC_DP_ALTMODE is enabled. Hi Markus, thanks for your feedback. I'll update the commit message to say "null pointer dereference" when I upload a v3 series. Thanks, Jameson
On Mon, Apr 29, 2024 at 05:43:16PM -0700, Jameson Thies wrote: > Hi Dmitry, > what are your thoughts on Abhishek's comment? I think we should > attempt to register the alternate mode when CONFIG_TYPEC_DP_ALTMODE is > not enabled. It would give us a more accurate representation of the > partner in user space. I understand your point about ignoring a > potential EOPNOTSUPP response from the non-stub function. What if we > leave ucsi.c alone, and replace the stub function's null return with a > call to typec_port_register_altmode(). That would register DP altmode > as an unsupported mode when CONFIG_TYPEC_DP_ALTMODE is not enabled, > and fix the null pointer dereference. But, it won't change behavior > when CONFIG_TYPEC_DP_ALTMODE is enabled. Yes, this sounds like a perfect idea! > > Hi Markus, > thanks for your feedback. I'll update the commit message to say "null > pointer dereference" when I upload a v3 series. > > Thanks, > Jameson
Great! I'll post a v3 series applying this shortly. Thanks, Jameson
On Wed, Apr 24, 2024 at 01:48:18AM +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. This results in a null deref when > trace_ucsi_register_altmode is called. Return an error from > ucsi_register_displayport when it is not supported and register the > altmode with typec_port_register_altmode. > > Reviewed-by: Jameson Thies <jthies@google.com> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org> This is not the correct way to fix this. The normal thing to do would be to add NULL checks. https://staticthinking.wordpress.com/2022/08/01/mixing-error-pointers-and-null/' regards, dan carpenter
Hi Dan, thank you for the reference. In this case I think returning NULL from ucsi_register_displayport stub and using a NULL check to prevent a NULL pointer dereference won't give us the desired behavior. When CONFIG_TYPEC_DP_ALTMODE is not enabled, the function would return NULL and the UCSI driver will never end up registering DisplayPort alternate mode. I'll update the commit message to note that this patch adds a fallback registration for DisplayPort alternate mode in addition to simply fixing the NULL pointer dereference caused by returning it in the ucsi_register_displayport stub. Separately we could add a NULL check, but I don't think it's necessary. Neither the non-stub ucsi_register_displayport or typec_port_register_altmode look like they will return NULL. Thanks, Jameson
It looks like in v3 you changed this to: - return NULL; + return typec_port_register_altmode(con->port, desc); Which is fine. Returning a special error pointer which means success is the part that I objected to. regards, dan carpenter
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c index cb52e7b0a2c5c..f3b413f94fd28 100644 --- a/drivers/usb/typec/ucsi/ucsi.c +++ b/drivers/usb/typec/ucsi/ucsi.c @@ -361,6 +361,9 @@ static int ucsi_register_altmode(struct ucsi_connector *con, switch (desc->svid) { case USB_TYPEC_DP_SID: alt = ucsi_register_displayport(con, override, i, desc); + if (IS_ERR(alt) && PTR_ERR(alt) == -EOPNOTSUPP) + alt = typec_port_register_altmode(con->port, desc); + break; case USB_TYPEC_NVIDIA_VLINK_SID: if (desc->vdo == USB_TYPEC_NVIDIA_VLINK_DBG_VDO) diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h index c4d103db9d0f8..c663dce0659ee 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 ERR_PTR(-EOPNOTSUPP); } static inline void