| Message ID | 20211217140327.31921-1-heikki.krogerus@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: typec: ucsi: Only check the contract if there is a connection | expand |
Lo! Thx for working out a fix this quickly! I'm just the regression tracker, but I think there are a few minor details to improve here. On 17.12.21 15:03, Heikki Krogerus wrote: > The driver must make sure there is an actual connection > before checking details about the USB Power Delivery > contract. Those details are not valid unless there is a > connection. > > This fixes NULL pointer dereference that is caused by an > attempt to register bogus partner alternate mode that the > firmware on some platform may report before the actual > connection. > > Fixes: 6cbe4b2d5a3f ("usb: typec: ucsi: Check the partner alt modes always if there is PD contract") > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=215117 BugLink? Is that a tag we really use? Then I'm unaware of it. Greg is the expert and can likely clarify, but that line afaik needs to replaced by this: Link: https://bugzilla.kernel.org/show_bug.cgi?id=215117 Link: https://lore.kernel.org/linux-usb/bug-215117-208809@https.bugzilla.kernel.org%2F/ Normally the last line would need a 's!linux-usb!r!', but seems the kernel.org redirector doesn't work well in this particular case, so I guess it's better this way than not at all :-/ The second line will also make the regression tracking bot automatically close the issue (but I fear it might also fail due to the slash at the end of the message-id :-/) I think this line should be there as well: Reported-by: Chris Hixon <linux-kernel-bugs@hixontech.com> But according to Documentation/process/5.Posting.rst you first need to ask Chris if he's okay with that. Ciao, Thorsten #regzbot ^backmonitor: https://lore.kernel.org/linux-usb/bug-215117-208809@https.bugzilla.kernel.org%2F/ > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > drivers/usb/typec/ucsi/ucsi.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c > index 9d6b7e02d6efb..f0c2fa19f3e0f 100644 > --- a/drivers/usb/typec/ucsi/ucsi.c > +++ b/drivers/usb/typec/ucsi/ucsi.c > @@ -1164,7 +1164,9 @@ static int ucsi_register_port(struct ucsi *ucsi, int index) > ret = 0; > } > > - if (UCSI_CONSTAT_PWR_OPMODE(con->status.flags) == UCSI_CONSTAT_PWR_OPMODE_PD) { > + if (con->partner && > + UCSI_CONSTAT_PWR_OPMODE(con->status.flags) == > + UCSI_CONSTAT_PWR_OPMODE_PD) { > ucsi_get_src_pdos(con); > ucsi_check_altmodes(con); > } >
Hi, On Fri, Dec 17, 2021 at 03:32:59PM +0100, Thorsten Leemhuis wrote: > Lo! Thx for working out a fix this quickly! > > I'm just the regression tracker, but I think there are a few minor > details to improve here. > > On 17.12.21 15:03, Heikki Krogerus wrote: > > The driver must make sure there is an actual connection > > before checking details about the USB Power Delivery > > contract. Those details are not valid unless there is a > > connection. > > > > This fixes NULL pointer dereference that is caused by an > > attempt to register bogus partner alternate mode that the > > firmware on some platform may report before the actual > > connection. > > > > Fixes: 6cbe4b2d5a3f ("usb: typec: ucsi: Check the partner alt modes always if there is PD contract") > > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=215117 > BugLink? Is that a tag we really use? Then I'm unaware of it. Greg is > the expert and can likely clarify, but that line afaik needs to replaced > by this: Although not yet documented, it is the appropriate tag for the link to the bug. It makes it clear that the link is to the bug and not to the discussion on the list. > Link: https://bugzilla.kernel.org/show_bug.cgi?id=215117 > Link: > https://lore.kernel.org/linux-usb/bug-215117-208809@https.bugzilla.kernel.org%2F/ > > Normally the last line would need a 's!linux-usb!r!', but seems the > kernel.org redirector doesn't work well in this particular case, so I > guess it's better this way than not at all :-/ > > The second line will also make the regression tracking bot automatically > close the issue (but I fear it might also fail due to the slash at the > end of the message-id :-/) Greg will add the "Link" tag to the commit when, and if, he actually takes the patch. I do not add it because I do not want any bots to react to the patch before it has actually been accepted. The bug shouldn't be closed before the fix has really been accepted. > I think this line should be there as well: > > Reported-by: Chris Hixon <linux-kernel-bugs@hixontech.com> +Chris This is true. I'll add the Reported-by tag if it's OK to you Chris? thanks,
On 17.12.21 16:14, Heikki Krogerus wrote: > Hi, > > On Fri, Dec 17, 2021 at 03:32:59PM +0100, Thorsten Leemhuis wrote: >> Lo! Thx for working out a fix this quickly! >> >> I'm just the regression tracker, but I think there are a few minor >> details to improve here. >> >> On 17.12.21 15:03, Heikki Krogerus wrote: >>> The driver must make sure there is an actual connection >>> before checking details about the USB Power Delivery >>> contract. Those details are not valid unless there is a >>> connection. >>> >>> This fixes NULL pointer dereference that is caused by an >>> attempt to register bogus partner alternate mode that the >>> firmware on some platform may report before the actual >>> connection. >>> >>> Fixes: 6cbe4b2d5a3f ("usb: typec: ucsi: Check the partner alt modes always if there is PD contract") >>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=215117 >> BugLink? Is that a tag we really use? Then I'm unaware of it. Greg is >> the expert and can likely clarify, but that line afaik needs to replaced >> by this: > > Although not yet documented, it is the appropriate tag for the link to > the bug. For you maybe. But it kind of becomes a mess if various people create different tags, as they do now (you are just one of them). > It makes it clear that the link is to the bug and not to > the discussion on the list. I agree that some clarification is needed, that's why I recently proposed something: https://lore.kernel.org/lkml/cover.1639042966.git.linux@leemhuis.info/ Maybe chime in there. >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=215117 >> Link: >> https://lore.kernel.org/linux-usb/bug-215117-208809@https.bugzilla.kernel.org%2F/ >> >> Normally the last line would need a 's!linux-usb!r!', but seems the >> kernel.org redirector doesn't work well in this particular case, so I >> guess it's better this way than not at all :-/ >> >> The second line will also make the regression tracking bot automatically >> close the issue (but I fear it might also fail due to the slash at the >> end of the message-id :-/) > > Greg will add the "Link" tag to the commit when, and if, he actually > takes the patch. No, that is another Link tag. Let me quote Documentation/process/submitting-patches.rst: ``` If related discussions or any other background information behind the change can be found on the web, add 'Link:' tags pointing to it. In case your patch fixes a bug, for example, add a tag with a URL referencing the report in the mailing list archives or a bug tracker; ``` This concept is old, but the text was reworked recently to make this use case for the Link: tag clearer. For details see: https://git.kernel.org/linus/1f57bd42b77c As the issue was discussed in a bug tracker and on the list, please add Link tags to both places. > I do not add it because I do not want any bots to > react to the patch before it has actually been accepted. > > The bug shouldn't be closed before the fix has really been accepted. > >> I think this line should be there as well: >> >> Reported-by: Chris Hixon <linux-kernel-bugs@hixontech.com> > > +Chris > > This is true. I'll add the Reported-by tag if it's OK to you Chris? thx! Ciao, Thorsten
On 12/17/21 8:14 AM, Heikki Krogerus wrote: > Hi, > > On Fri, Dec 17, 2021 at 03:32:59PM +0100, Thorsten Leemhuis wrote: >> Lo! Thx for working out a fix this quickly! >> >> I'm just the regression tracker, but I think there are a few minor >> details to improve here. >> >> On 17.12.21 15:03, Heikki Krogerus wrote: >>> The driver must make sure there is an actual connection >>> before checking details about the USB Power Delivery >>> contract. Those details are not valid unless there is a >>> connection. >>> >>> This fixes NULL pointer dereference that is caused by an >>> attempt to register bogus partner alternate mode that the >>> firmware on some platform may report before the actual >>> connection. >>> >>> Fixes: 6cbe4b2d5a3f ("usb: typec: ucsi: Check the partner alt modes always if there is PD contract") >>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=215117 >> BugLink? Is that a tag we really use? Then I'm unaware of it. Greg is >> the expert and can likely clarify, but that line afaik needs to replaced >> by this: > Although not yet documented, it is the appropriate tag for the link to > the bug. It makes it clear that the link is to the bug and not to > the discussion on the list. > >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=215117 >> Link: >> https://lore.kernel.org/linux-usb/bug-215117-208809@https.bugzilla.kernel.org%2F/ >> >> Normally the last line would need a 's!linux-usb!r!', but seems the >> kernel.org redirector doesn't work well in this particular case, so I >> guess it's better this way than not at all :-/ >> >> The second line will also make the regression tracking bot automatically >> close the issue (but I fear it might also fail due to the slash at the >> end of the message-id :-/) > Greg will add the "Link" tag to the commit when, and if, he actually > takes the patch. I do not add it because I do not want any bots to > react to the patch before it has actually been accepted. > > The bug shouldn't be closed before the fix has really been accepted. > >> I think this line should be there as well: >> >> Reported-by: Chris Hixon <linux-kernel-bugs@hixontech.com> > +Chris > > This is true. I'll add the Reported-by tag if it's OK to you Chris? > > thanks, > Yes, that's fine with me. Please add the tag: Reported-by: Chris Hixon <linux-kernel-bugs@hixontech.com> Thanks
On Fri, Dec 17, 2021 at 05:14:42PM +0200, Heikki Krogerus wrote: > Hi, > > On Fri, Dec 17, 2021 at 03:32:59PM +0100, Thorsten Leemhuis wrote: > > Lo! Thx for working out a fix this quickly! > > > > I'm just the regression tracker, but I think there are a few minor > > details to improve here. > > > > On 17.12.21 15:03, Heikki Krogerus wrote: > > > The driver must make sure there is an actual connection > > > before checking details about the USB Power Delivery > > > contract. Those details are not valid unless there is a > > > connection. > > > > > > This fixes NULL pointer dereference that is caused by an > > > attempt to register bogus partner alternate mode that the > > > firmware on some platform may report before the actual > > > connection. > > > > > > Fixes: 6cbe4b2d5a3f ("usb: typec: ucsi: Check the partner alt modes always if there is PD contract") > > > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=215117 > > BugLink? Is that a tag we really use? Then I'm unaware of it. Greg is > > the expert and can likely clarify, but that line afaik needs to replaced > > by this: > > Although not yet documented, it is the appropriate tag for the link to > the bug. It makes it clear that the link is to the bug and not to > the discussion on the list. > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=215117 > > Link: > > https://lore.kernel.org/linux-usb/bug-215117-208809@https.bugzilla.kernel.org%2F/ > > > > Normally the last line would need a 's!linux-usb!r!', but seems the > > kernel.org redirector doesn't work well in this particular case, so I > > guess it's better this way than not at all :-/ > > > > The second line will also make the regression tracking bot automatically > > close the issue (but I fear it might also fail due to the slash at the > > end of the message-id :-/) > > Greg will add the "Link" tag to the commit when, and if, he actually > takes the patch. I do not add it because I do not want any bots to > react to the patch before it has actually been accepted. The link my scripts add is to the post on lore.kernel.org, not to anywhere else. So if this needs to point to a different report, like Thorsten shows above, then that needs to be done "by hand". I guess I'll go add these on my own :( greg k-h
On Fri, Dec 17, 2021 at 06:25:09PM +0100, Thorsten Leemhuis wrote: > > > On 17.12.21 16:14, Heikki Krogerus wrote: > > Hi, > > > > On Fri, Dec 17, 2021 at 03:32:59PM +0100, Thorsten Leemhuis wrote: > >> Lo! Thx for working out a fix this quickly! > >> > >> I'm just the regression tracker, but I think there are a few minor > >> details to improve here. > >> > >> On 17.12.21 15:03, Heikki Krogerus wrote: > >>> The driver must make sure there is an actual connection > >>> before checking details about the USB Power Delivery > >>> contract. Those details are not valid unless there is a > >>> connection. > >>> > >>> This fixes NULL pointer dereference that is caused by an > >>> attempt to register bogus partner alternate mode that the > >>> firmware on some platform may report before the actual > >>> connection. > >>> > >>> Fixes: 6cbe4b2d5a3f ("usb: typec: ucsi: Check the partner alt modes always if there is PD contract") > >>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=215117 > >> BugLink? Is that a tag we really use? Then I'm unaware of it. Greg is > >> the expert and can likely clarify, but that line afaik needs to replaced > >> by this: > > > > Although not yet documented, it is the appropriate tag for the link to > > the bug. > > For you maybe. But it kind of becomes a mess if various people create > different tags, as they do now (you are just one of them). > > > It makes it clear that the link is to the bug and not to > > the discussion on the list. > > I agree that some clarification is needed, that's why I recently > proposed something: > https://lore.kernel.org/lkml/cover.1639042966.git.linux@leemhuis.info/ Hashtags would be fine by me. I'm using the "BugLink" tag only because it has been commonly used so far. > Maybe chime in there. > > >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=215117 > >> Link: > >> https://lore.kernel.org/linux-usb/bug-215117-208809@https.bugzilla.kernel.org%2F/ > >> > >> Normally the last line would need a 's!linux-usb!r!', but seems the > >> kernel.org redirector doesn't work well in this particular case, so I > >> guess it's better this way than not at all :-/ > >> > >> The second line will also make the regression tracking bot automatically > >> close the issue (but I fear it might also fail due to the slash at the > >> end of the message-id :-/) > > > > Greg will add the "Link" tag to the commit when, and if, he actually > > takes the patch. > > No, that is another Link tag. Let me quote > Documentation/process/submitting-patches.rst: > > ``` > If related discussions or any other background information behind the > change can be found on the web, add 'Link:' tags pointing to it. In case > your patch fixes a bug, for example, add a tag with a URL referencing > the report in the mailing list archives or a bug tracker; > ``` > > This concept is old, but the text was reworked recently to make this use > case for the Link: tag clearer. > For details see: https://git.kernel.org/linus/1f57bd42b77c > > As the issue was discussed in a bug tracker and on the list, please add > Link tags to both places. Fair enough. thanks,
On Mon, Dec 20, 2021 at 03:47:56PM +0100, Greg Kroah-Hartman wrote: > On Fri, Dec 17, 2021 at 05:14:42PM +0200, Heikki Krogerus wrote: > > Hi, > > > > On Fri, Dec 17, 2021 at 03:32:59PM +0100, Thorsten Leemhuis wrote: > > > Lo! Thx for working out a fix this quickly! > > > > > > I'm just the regression tracker, but I think there are a few minor > > > details to improve here. > > > > > > On 17.12.21 15:03, Heikki Krogerus wrote: > > > > The driver must make sure there is an actual connection > > > > before checking details about the USB Power Delivery > > > > contract. Those details are not valid unless there is a > > > > connection. > > > > > > > > This fixes NULL pointer dereference that is caused by an > > > > attempt to register bogus partner alternate mode that the > > > > firmware on some platform may report before the actual > > > > connection. > > > > > > > > Fixes: 6cbe4b2d5a3f ("usb: typec: ucsi: Check the partner alt modes always if there is PD contract") > > > > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=215117 > > > BugLink? Is that a tag we really use? Then I'm unaware of it. Greg is > > > the expert and can likely clarify, but that line afaik needs to replaced > > > by this: > > > > Although not yet documented, it is the appropriate tag for the link to > > the bug. It makes it clear that the link is to the bug and not to > > the discussion on the list. > > > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=215117 > > > Link: > > > https://lore.kernel.org/linux-usb/bug-215117-208809@https.bugzilla.kernel.org%2F/ > > > > > > Normally the last line would need a 's!linux-usb!r!', but seems the > > > kernel.org redirector doesn't work well in this particular case, so I > > > guess it's better this way than not at all :-/ > > > > > > The second line will also make the regression tracking bot automatically > > > close the issue (but I fear it might also fail due to the slash at the > > > end of the message-id :-/) > > > > Greg will add the "Link" tag to the commit when, and if, he actually > > takes the patch. I do not add it because I do not want any bots to > > react to the patch before it has actually been accepted. > > The link my scripts add is to the post on lore.kernel.org, not to > anywhere else. > > So if this needs to point to a different report, like Thorsten shows > above, then that needs to be done "by hand". > > I guess I'll go add these on my own :( OK... so I don't prepare v2 then. thanks,
On Mon, Dec 20, 2021 at 05:41:22PM +0200, Heikki Krogerus wrote: > On Mon, Dec 20, 2021 at 03:47:56PM +0100, Greg Kroah-Hartman wrote: > > On Fri, Dec 17, 2021 at 05:14:42PM +0200, Heikki Krogerus wrote: > > > Hi, > > > > > > On Fri, Dec 17, 2021 at 03:32:59PM +0100, Thorsten Leemhuis wrote: > > > > Lo! Thx for working out a fix this quickly! > > > > > > > > I'm just the regression tracker, but I think there are a few minor > > > > details to improve here. > > > > > > > > On 17.12.21 15:03, Heikki Krogerus wrote: > > > > > The driver must make sure there is an actual connection > > > > > before checking details about the USB Power Delivery > > > > > contract. Those details are not valid unless there is a > > > > > connection. > > > > > > > > > > This fixes NULL pointer dereference that is caused by an > > > > > attempt to register bogus partner alternate mode that the > > > > > firmware on some platform may report before the actual > > > > > connection. > > > > > > > > > > Fixes: 6cbe4b2d5a3f ("usb: typec: ucsi: Check the partner alt modes always if there is PD contract") > > > > > BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=215117 > > > > BugLink? Is that a tag we really use? Then I'm unaware of it. Greg is > > > > the expert and can likely clarify, but that line afaik needs to replaced > > > > by this: > > > > > > Although not yet documented, it is the appropriate tag for the link to > > > the bug. It makes it clear that the link is to the bug and not to > > > the discussion on the list. > > > > > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=215117 > > > > Link: > > > > https://lore.kernel.org/linux-usb/bug-215117-208809@https.bugzilla.kernel.org%2F/ > > > > > > > > Normally the last line would need a 's!linux-usb!r!', but seems the > > > > kernel.org redirector doesn't work well in this particular case, so I > > > > guess it's better this way than not at all :-/ > > > > > > > > The second line will also make the regression tracking bot automatically > > > > close the issue (but I fear it might also fail due to the slash at the > > > > end of the message-id :-/) > > > > > > Greg will add the "Link" tag to the commit when, and if, he actually > > > takes the patch. I do not add it because I do not want any bots to > > > react to the patch before it has actually been accepted. > > > > The link my scripts add is to the post on lore.kernel.org, not to > > anywhere else. > > > > So if this needs to point to a different report, like Thorsten shows > > above, then that needs to be done "by hand". > > > > I guess I'll go add these on my own :( > > OK... so I don't prepare v2 then. No, please do, I will not get to this for another few days because it takes me more work :)
On Mon, Dec 20, 2021 at 05:55:14PM +0100, Greg Kroah-Hartman wrote: > > OK... so I don't prepare v2 then. > > No, please do, I will not get to this for another few days because it > takes me more work :) OK. I'll send it asap, today. thanks,
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c index 9d6b7e02d6efb..f0c2fa19f3e0f 100644 --- a/drivers/usb/typec/ucsi/ucsi.c +++ b/drivers/usb/typec/ucsi/ucsi.c @@ -1164,7 +1164,9 @@ static int ucsi_register_port(struct ucsi *ucsi, int index) ret = 0; } - if (UCSI_CONSTAT_PWR_OPMODE(con->status.flags) == UCSI_CONSTAT_PWR_OPMODE_PD) { + if (con->partner && + UCSI_CONSTAT_PWR_OPMODE(con->status.flags) == + UCSI_CONSTAT_PWR_OPMODE_PD) { ucsi_get_src_pdos(con); ucsi_check_altmodes(con); }
The driver must make sure there is an actual connection before checking details about the USB Power Delivery contract. Those details are not valid unless there is a connection. This fixes NULL pointer dereference that is caused by an attempt to register bogus partner alternate mode that the firmware on some platform may report before the actual connection. Fixes: 6cbe4b2d5a3f ("usb: typec: ucsi: Check the partner alt modes always if there is PD contract") BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=215117 Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> --- drivers/usb/typec/ucsi/ucsi.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)