| Message ID | 20221103145450.1409273-2-jsd@semihalf.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | char: tpm: Adjust cr50_i2c locking mechanism | expand |
On Thu, Nov 03, 2022 at 03:54:48PM +0100, Jan Dabros wrote: > Currently tpm transactions are executed unconditionally in > tpm_pm_suspend() function, what may lead to races with other tpm > accessors in the system. > > Add proper locking mechanisms by calling tpm_try_get_ops() which is a > wrapper on tpm_chip_start(). > > Signed-off-by: Jan Dabros <jsd@semihalf.com> AFAIK processes are freezed before suspend callbacks are called, and the callbacks are called sequentially. I have no idea what is meant by "TPM accessor" here. Please describe the concurrency scenario in the commit message where the race could happen, if it is hard to reproduce, and add an appropriate fixes tag. BR, Jarkko
niedz., 6 lis 2022 o 20:49 Jarkko Sakkinen <jarkko@kernel.org> napisał(a): > > On Thu, Nov 03, 2022 at 03:54:48PM +0100, Jan Dabros wrote: > > Currently tpm transactions are executed unconditionally in > > tpm_pm_suspend() function, what may lead to races with other tpm > > accessors in the system. > > > > Add proper locking mechanisms by calling tpm_try_get_ops() which is a > > wrapper on tpm_chip_start(). > > > > Signed-off-by: Jan Dabros <jsd@semihalf.com> > > AFAIK processes are freezed before suspend callbacks are called, and > the callbacks are called sequentially. I have no idea what is meant > by "TPM accessor" here. User space processes are freezed before suspend, but kernel threads are not freezable by default. In my particular case it was a hwrng thread started from drivers/char/hw_random/core.c - I was referring to it as "TPM accessor". For sure I should be more precise in a commit msg. > Please describe the concurrency scenario in the commit message where the > race could happen, if it is hard to reproduce, and add an appropriate fixes > tag. I will describe my scenario in more detail in the next version. Regarding the "fixes" tag - I'm not too familiar with it, but looking at the kernel submission guide, "fixes" should be used either when there was a particular commit in the past which introduced the bug or if a patch fixes an already logged bug entry (so that one can paste URL). In my case both are not applicable, so please advise what exactly I should put after this tag? Best Regards, Jan
On Mon, Nov 07, 2022 at 09:45:41AM +0100, Jan Dąbroś wrote: > niedz., 6 lis 2022 o 20:49 Jarkko Sakkinen <jarkko@kernel.org> napisał(a): > > > > On Thu, Nov 03, 2022 at 03:54:48PM +0100, Jan Dabros wrote: > > > Currently tpm transactions are executed unconditionally in > > > tpm_pm_suspend() function, what may lead to races with other tpm > > > accessors in the system. > > > > > > Add proper locking mechanisms by calling tpm_try_get_ops() which is a > > > wrapper on tpm_chip_start(). > > > > > > Signed-off-by: Jan Dabros <jsd@semihalf.com> > > > > AFAIK processes are freezed before suspend callbacks are called, and > > the callbacks are called sequentially. I have no idea what is meant > > by "TPM accessor" here. > > User space processes are freezed before suspend, but kernel threads > are not freezable by default. In my particular case it was a hwrng > thread started from drivers/char/hw_random/core.c - I was referring to > it as "TPM accessor". For sure I should be more precise in a commit > msg. OK, great. > > > Please describe the concurrency scenario in the commit message where the > > race could happen, if it is hard to reproduce, and add an appropriate fixes > > tag. > > I will describe my scenario in more detail in the next version. > Regarding the "fixes" tag - I'm not too familiar with it, but looking > at the kernel submission guide, "fixes" should be used either when > there was a particular commit in the past which introduced the bug or > if a patch fixes an already logged bug entry (so that one can paste > URL). In my case both are not applicable, so please advise what > exactly I should put after this tag? It is Fixed: <12 character prefix of the hash> ("short summary") It should point out to the commit, which introduced the issue/bug. > Best Regards, > Jan BR, Jarkko
Hi Jarkko, On Mon, Nov 07, 2022 at 06:35:25PM +0200, Jarkko Sakkinen wrote: > On Mon, Nov 07, 2022 at 09:45:41AM +0100, Jan Dąbroś wrote: > > niedz., 6 lis 2022 o 20:49 Jarkko Sakkinen <jarkko@kernel.org> napisał(a): > > > > > > On Thu, Nov 03, 2022 at 03:54:48PM +0100, Jan Dabros wrote: > > > > Currently tpm transactions are executed unconditionally in > > > > tpm_pm_suspend() function, what may lead to races with other tpm > > > > accessors in the system. > > > > > > > > Add proper locking mechanisms by calling tpm_try_get_ops() which is a > > > > wrapper on tpm_chip_start(). > > > > > > > > Signed-off-by: Jan Dabros <jsd@semihalf.com> > > > > > > AFAIK processes are freezed before suspend callbacks are called, and > > > the callbacks are called sequentially. I have no idea what is meant > > > by "TPM accessor" here. > > > > User space processes are freezed before suspend, but kernel threads > > are not freezable by default. In my particular case it was a hwrng > > thread started from drivers/char/hw_random/core.c - I was referring to > > it as "TPM accessor". For sure I should be more precise in a commit > > msg. > > OK, great. > > > > > > Please describe the concurrency scenario in the commit message where the > > > race could happen, if it is hard to reproduce, and add an appropriate fixes > > > tag. > > > > I will describe my scenario in more detail in the next version. > > Regarding the "fixes" tag - I'm not too familiar with it, but looking > > at the kernel submission guide, "fixes" should be used either when > > there was a particular commit in the past which introduced the bug or > > if a patch fixes an already logged bug entry (so that one can paste > > URL). In my case both are not applicable, so please advise what > > exactly I should put after this tag? > > It is > > Fixed: <12 character prefix of the hash> ("short summary") > > It should point out to the commit, which introduced the issue/bug. Somebody just reported a bug to me in hwrng stuff, which I traced down to this lack of tpm_try_get_ops(), and then I coded up the exact same patch as Jan's here, saw it fixed the problem, and then realized it was already submitted in that exact form. Because of some recent hwrng changes, this bug will be triggered more often than it was before. So, even though Jan hasn't submitted a v+1, do you think you could take this 1/3 commit, fix up the commit message or whatever you need there, and get this in for 6.1-rc8? It's an important fix. Thanks, Jason
On Mon, Nov 28, 2022 at 6:04 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > > Hi Jarkko, > > On Mon, Nov 07, 2022 at 06:35:25PM +0200, Jarkko Sakkinen wrote: > > On Mon, Nov 07, 2022 at 09:45:41AM +0100, Jan Dąbroś wrote: > > > niedz., 6 lis 2022 o 20:49 Jarkko Sakkinen <jarkko@kernel.org> napisał(a): > > > > > > > > On Thu, Nov 03, 2022 at 03:54:48PM +0100, Jan Dabros wrote: > > > > > Currently tpm transactions are executed unconditionally in > > > > > tpm_pm_suspend() function, what may lead to races with other tpm > > > > > accessors in the system. > > > > > > > > > > Add proper locking mechanisms by calling tpm_try_get_ops() which is a > > > > > wrapper on tpm_chip_start(). > > > > > > > > > > Signed-off-by: Jan Dabros <jsd@semihalf.com> > > > > > > > > AFAIK processes are freezed before suspend callbacks are called, and > > > > the callbacks are called sequentially. I have no idea what is meant > > > > by "TPM accessor" here. > > > > > > User space processes are freezed before suspend, but kernel threads > > > are not freezable by default. In my particular case it was a hwrng > > > thread started from drivers/char/hw_random/core.c - I was referring to > > > it as "TPM accessor". For sure I should be more precise in a commit > > > msg. > > > > OK, great. > > > > > > > > > Please describe the concurrency scenario in the commit message where the > > > > race could happen, if it is hard to reproduce, and add an appropriate fixes > > > > tag. > > > > > > I will describe my scenario in more detail in the next version. > > > Regarding the "fixes" tag - I'm not too familiar with it, but looking > > > at the kernel submission guide, "fixes" should be used either when > > > there was a particular commit in the past which introduced the bug or > > > if a patch fixes an already logged bug entry (so that one can paste > > > URL). In my case both are not applicable, so please advise what > > > exactly I should put after this tag? > > > > It is > > > > Fixed: <12 character prefix of the hash> ("short summary") > > > > It should point out to the commit, which introduced the issue/bug. > > Somebody just reported a bug to me in hwrng stuff, which I traced down > to this lack of tpm_try_get_ops(), and then I coded up the exact same > patch as Jan's here, saw it fixed the problem, and then realized it was > already submitted in that exact form. > > Because of some recent hwrng changes, this bug will be triggered more > often than it was before. > > So, even though Jan hasn't submitted a v+1, do you think you could take > this 1/3 commit, fix up the commit message or whatever you need there, > and get this in for 6.1-rc8? It's an important fix. > > Thanks, > Jason And if it helps, here's some more metadata: Tested-by: Jason A. Donenfeld <Jason@zx2c4.com> Reported-by: Vlastimil Babka <vbabka@suse.cz> Link: https://lore.kernel.org/all/c5ba47ef-393f-1fba-30bd-1230d1b4b592@suse.cz/ Cc: stable@vger.kernel.org
On 11/28/22 18:04, Jason A. Donenfeld wrote: > Hi Jarkko, > > On Mon, Nov 07, 2022 at 06:35:25PM +0200, Jarkko Sakkinen wrote: >> On Mon, Nov 07, 2022 at 09:45:41AM +0100, Jan Dąbroś wrote: >> > niedz., 6 lis 2022 o 20:49 Jarkko Sakkinen <jarkko@kernel.org> napisał(a): >> > > >> > > On Thu, Nov 03, 2022 at 03:54:48PM +0100, Jan Dabros wrote: >> > > > Currently tpm transactions are executed unconditionally in >> > > > tpm_pm_suspend() function, what may lead to races with other tpm >> > > > accessors in the system. >> > > > >> > > > Add proper locking mechanisms by calling tpm_try_get_ops() which is a >> > > > wrapper on tpm_chip_start(). >> > > > >> > > > Signed-off-by: Jan Dabros <jsd@semihalf.com> >> > > >> > > AFAIK processes are freezed before suspend callbacks are called, and >> > > the callbacks are called sequentially. I have no idea what is meant >> > > by "TPM accessor" here. >> > >> > User space processes are freezed before suspend, but kernel threads >> > are not freezable by default. In my particular case it was a hwrng >> > thread started from drivers/char/hw_random/core.c - I was referring to >> > it as "TPM accessor". For sure I should be more precise in a commit >> > msg. >> >> OK, great. >> >> > >> > > Please describe the concurrency scenario in the commit message where the >> > > race could happen, if it is hard to reproduce, and add an appropriate fixes >> > > tag. >> > >> > I will describe my scenario in more detail in the next version. >> > Regarding the "fixes" tag - I'm not too familiar with it, but looking >> > at the kernel submission guide, "fixes" should be used either when >> > there was a particular commit in the past which introduced the bug or >> > if a patch fixes an already logged bug entry (so that one can paste >> > URL). In my case both are not applicable, so please advise what >> > exactly I should put after this tag? >> >> It is >> >> Fixed: <12 character prefix of the hash> ("short summary") >> >> It should point out to the commit, which introduced the issue/bug. > > Somebody just reported a bug to me in hwrng stuff, which I traced down > to this lack of tpm_try_get_ops(), and then I coded up the exact same > patch as Jan's here, saw it fixed the problem, and then realized it was > already submitted in that exact form. > > Because of some recent hwrng changes, this bug will be triggered more > often than it was before. > > So, even though Jan hasn't submitted a v+1, do you think you could take > this 1/3 commit, fix up the commit message or whatever you need there, > and get this in for 6.1-rc8? It's an important fix. For the record, the bug report is here: https://lore.kernel.org/all/c5ba47ef-393f-1fba-30bd-1230d1b4b592@suse.cz/ (patch author Jan and TPM maintainers were Cc'd) FWIW I tested the patch 1/3 on top of rc7 and didn't reproduce the issue. But also had no luck of reproducing it on plain rc7 yet, so it doesn't say much, unfortunately. > Thanks, > Jason
On 11/28/22 18:07, Jason A. Donenfeld wrote: > On Mon, Nov 28, 2022 at 6:04 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: >> > > >> > > > Please describe the concurrency scenario in the commit message where the >> > > > race could happen, if it is hard to reproduce, and add an appropriate fixes >> > > > tag. >> > > >> > > I will describe my scenario in more detail in the next version. >> > > Regarding the "fixes" tag - I'm not too familiar with it, but looking >> > > at the kernel submission guide, "fixes" should be used either when >> > > there was a particular commit in the past which introduced the bug or >> > > if a patch fixes an already logged bug entry (so that one can paste >> > > URL). In my case both are not applicable, so please advise what >> > > exactly I should put after this tag? >> > >> > It is >> > >> > Fixed: <12 character prefix of the hash> ("short summary") >> > >> > It should point out to the commit, which introduced the issue/bug. >> >> Somebody just reported a bug to me in hwrng stuff, which I traced down >> to this lack of tpm_try_get_ops(), and then I coded up the exact same >> patch as Jan's here, saw it fixed the problem, and then realized it was >> already submitted in that exact form. >> >> Because of some recent hwrng changes, this bug will be triggered more >> often than it was before. >> >> So, even though Jan hasn't submitted a v+1, do you think you could take >> this 1/3 commit, fix up the commit message or whatever you need there, >> and get this in for 6.1-rc8? It's an important fix. >> >> Thanks, >> Jason > > And if it helps, here's some more metadata: > > Tested-by: Jason A. Donenfeld <Jason@zx2c4.com> > Reported-by: Vlastimil Babka <vbabka@suse.cz> You can make that: Reported-and-tested-by: Vlastimil Babka <vbabka@suse.cz> > Link: https://lore.kernel.org/all/c5ba47ef-393f-1fba-30bd-1230d1b4b592@suse.cz/ > Cc: stable@vger.kernel.org Not an expert here but looks like a valid Fixes: would be Fixes: e891db1a18bf ("tpm: turn on TPM on suspend for TPM 1.x") The 6.1-rc1 commit 3b006c439d58d ("hwrng: core - start hwrng kthread also for untrusted sources") likely exposed the bug in my case, but it wouldn't be the real root cause. But it makes it prudent to get the fix to 6.1.
On Mon, Nov 28, 2022 at 8:46 PM Vlastimil Babka <vbabka@suse.cz> wrote: > > On 11/28/22 18:07, Jason A. Donenfeld wrote: > > On Mon, Nov 28, 2022 at 6:04 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote: > >> > > > >> > > > Please describe the concurrency scenario in the commit message where the > >> > > > race could happen, if it is hard to reproduce, and add an appropriate fixes > >> > > > tag. > >> > > > >> > > I will describe my scenario in more detail in the next version. > >> > > Regarding the "fixes" tag - I'm not too familiar with it, but looking > >> > > at the kernel submission guide, "fixes" should be used either when > >> > > there was a particular commit in the past which introduced the bug or > >> > > if a patch fixes an already logged bug entry (so that one can paste > >> > > URL). In my case both are not applicable, so please advise what > >> > > exactly I should put after this tag? > >> > > >> > It is > >> > > >> > Fixed: <12 character prefix of the hash> ("short summary") > >> > > >> > It should point out to the commit, which introduced the issue/bug. > >> > >> Somebody just reported a bug to me in hwrng stuff, which I traced down > >> to this lack of tpm_try_get_ops(), and then I coded up the exact same > >> patch as Jan's here, saw it fixed the problem, and then realized it was > >> already submitted in that exact form. > >> > >> Because of some recent hwrng changes, this bug will be triggered more > >> often than it was before. > >> > >> So, even though Jan hasn't submitted a v+1, do you think you could take > >> this 1/3 commit, fix up the commit message or whatever you need there, > >> and get this in for 6.1-rc8? It's an important fix. > >> > >> Thanks, > >> Jason > > > > And if it helps, here's some more metadata: > > > > Tested-by: Jason A. Donenfeld <Jason@zx2c4.com> > > Reported-by: Vlastimil Babka <vbabka@suse.cz> > > You can make that: > Reported-and-tested-by: Vlastimil Babka <vbabka@suse.cz> > > > Link: https://lore.kernel.org/all/c5ba47ef-393f-1fba-30bd-1230d1b4b592@suse.cz/ > > Cc: stable@vger.kernel.org > > Not an expert here but looks like a valid Fixes: would be > Fixes: e891db1a18bf ("tpm: turn on TPM on suspend for TPM 1.x") > > The 6.1-rc1 commit 3b006c439d58d ("hwrng: core - start hwrng kthread also > for untrusted sources") likely exposed the bug in my case, but it wouldn't > be the real root cause. But it makes it prudent to get the fix to 6.1. Since this thread went dormant, and we're already pretty close to the end of the cycle, I'm going to resend this with the commit message fixed, as Jarkko asked at the beginning of the month. Jan can then send whatever needs fixing in 2/3 and 3/3 whenever he gets around to it. I'll keep the original authorship intact of course. Jason
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c index 1621ce8187052..d69905233aff2 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -401,13 +401,14 @@ int tpm_pm_suspend(struct device *dev) !pm_suspend_via_firmware()) goto suspended; - if (!tpm_chip_start(chip)) { + rc = tpm_try_get_ops(chip); + if (!rc) { if (chip->flags & TPM_CHIP_FLAG_TPM2) tpm2_shutdown(chip, TPM2_SU_STATE); else rc = tpm1_pm_suspend(chip, tpm_suspend_pcr); - tpm_chip_stop(chip); + tpm_put_ops(chip); } suspended:
Currently tpm transactions are executed unconditionally in tpm_pm_suspend() function, what may lead to races with other tpm accessors in the system. Add proper locking mechanisms by calling tpm_try_get_ops() which is a wrapper on tpm_chip_start(). Signed-off-by: Jan Dabros <jsd@semihalf.com> --- drivers/char/tpm/tpm-interface.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)