Hi,
Fedora got two bug reports https://bugzilla.redhat.com/show_bug.cgi?id=1788653
https://bugzilla.redhat.com/show_bug.cgi?id=1788257 of a usercopy bug from
tpm:
[ 67.037526] usercopy: Kernel memory exposure attempt detected from wrapped address (offset 0, size 18446634686907596985)!
[ 67.037541] ------------[ cut here ]------------
[ 67.037543] kernel BUG at mm/usercopy.c:99!
[ 67.037550] invalid opcode: 0000 [#1] SMP PTI
[ 67.037553] CPU: 1 PID: 3277 Comm: tpm2-abrmd Not tainted 5.4.7-200.fc31.x86_64 #1
[ 67.037555] Hardware name: Dell Inc. Latitude 5580/0FH6CJ, BIOS 1.16.0 07/03/2019
[ 67.037562] RIP: 0010:usercopy_abort+0x77/0x79
[ 67.037565] Code: 4c 0f 45 de 51 4c 89 d1 48 c7 c2 e3 ce 35 b0 57 48 c7 c6 30 80 34 b0 48 c7 c7 a8 cf 35 b0 48 0f 45 f2 4c 89 da e8 50 6c e4 ff <0f> 0b 4c 89 e1 49 89 d8 44 89 ea 31 f6 48 29 c1 48 c7 c7 25 cf 35
[ 67.037567] RSP: 0018:ffffae5b42eabe48 EFLAGS: 00010246
[ 67.037570] RAX: 000000000000006d RBX: ffffffffffffffff RCX: 0000000000000000
[ 67.037572] RDX: 0000000000000000 RSI: ffff9c83b6257908 RDI: ffff9c83b6257908
[ 67.037574] RBP: ffff9c836686c0b9 R08: ffff9c83b6257908 R09: 000000000000007c
[ 67.037576] R10: ffffae5b42eabcf8 R11: 0000000000000000 R12: ffff9c836686c0ba
[ 67.037578] R13: 0000000000000001 R14: ffff9c836686c010 R15: ffff9c836686c0ba
[ 67.037580] FS: 00007fb2dbfff700(0000) GS:ffff9c83b6240000(0000) knlGS:0000000000000000
[ 67.037582] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 67.037584] CR2: 00007fc1137f3e00 CR3: 00000002205c4002 CR4: 00000000003606e0
[ 67.037586] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 67.037588] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 67.037589] Call Trace:
[ 67.037595] __check_object_size.cold+0x46/0x80
[ 67.037600] tpm_common_read+0x74/0x140
[ 67.037605] vfs_read+0x9d/0x150
[ 67.037610] ksys_read+0x5f/0xe0
[ 67.037615] do_syscall_64+0x5b/0x1a0
[ 67.037620] entry_SYSCALL_64_after_hwframe+0x44/0xa9
I think this is related to d23d12484307 ("tpm: fix invalid locking in NONBLOCKING mode")
Specifically, if tpm_try_get_ops fails I don't think we should be putting the error
code in priv->response_length since tpm_common_read doesn't seem to account for
negative errno values.
I don't have a reproducer since this was just what was reported to Fedora's bug
reporter but both reports happened after that commit landed in stable.
Thanks,
Laura
On 1/7/20 10:51 AM, Laura Abbott wrote:
> I think this is related to d23d12484307 ("tpm: fix invalid locking in
> NONBLOCKING mode")
> Specifically, if tpm_try_get_ops fails I don't think we should be
> putting the error
> code in priv->response_length since tpm_common_read doesn't seem to
> account for
> negative errno values.
>
> I don't have a reproducer since this was just what was reported to
> Fedora's bug
> reporter but both reports happened after that commit landed in stable.
Thank you for the report Laura. Fix is on the way.
--
Tadeusz
The priv->responce_length can hold the size of an response or
an negative error code, and the tpm_common_read() needs to handle
both cases correctly. Changed the type of responce_length to
signed and accounted for negative value in tpm_common_read()
Cc: [email protected]
Fixes: d23d12484307 ("tpm: fix invalid locking in NONBLOCKING mode")
Reported-by: Laura Abbott <[email protected]>
Signed-off-by: Tadeusz Struk <[email protected]>
---
drivers/char/tpm/tpm-dev-common.c | 2 +-
drivers/char/tpm/tpm-dev.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index b23b0b999232..87f449340202 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -130,7 +130,7 @@ ssize_t tpm_common_read(struct file *file, char __user *buf,
priv->response_read = true;
ret_size = min_t(ssize_t, size, priv->response_length);
- if (!ret_size) {
+ if (ret_size <= 0) {
priv->response_length = 0;
goto out;
}
diff --git a/drivers/char/tpm/tpm-dev.h b/drivers/char/tpm/tpm-dev.h
index 1089fc0bb290..f3742bcc73e3 100644
--- a/drivers/char/tpm/tpm-dev.h
+++ b/drivers/char/tpm/tpm-dev.h
@@ -14,7 +14,7 @@ struct file_priv {
struct work_struct timeout_work;
struct work_struct async_work;
wait_queue_head_t async_wait;
- size_t response_length;
+ ssize_t response_length;
bool response_read;
bool command_enqueued;
On Wed, 2020-01-08 at 17:58 +0200, Jarkko Sakkinen wrote:
> On Tue, 2020-01-07 at 14:04 -0800, Tadeusz Struk wrote:
> > The priv->responce_length can hold the size of an response or
> > an negative error code, and the tpm_common_read() needs to handle
> > both cases correctly. Changed the type of responce_length to
> > signed and accounted for negative value in tpm_common_read()
> >
> > Cc: [email protected]
> > Fixes: d23d12484307 ("tpm: fix invalid locking in NONBLOCKING mode")
> > Reported-by: Laura Abbott <[email protected]>
> > Signed-off-by: Tadeusz Struk <[email protected]>
>
> Reviewed-by: Jarkko Sakkinen <[email protected]>
>
> Adding to the next PR.
Applied but had to fix bunch of typos, missing punctaction and
missing parentheses in the commit message. Even checkpatch.pl
was complaining :-/
Thanks.
/Jarkko
On Tue, 2020-01-07 at 14:04 -0800, Tadeusz Struk wrote:
> The priv->responce_length can hold the size of an response or
> an negative error code, and the tpm_common_read() needs to handle
> both cases correctly. Changed the type of responce_length to
> signed and accounted for negative value in tpm_common_read()
>
> Cc: [email protected]
> Fixes: d23d12484307 ("tpm: fix invalid locking in NONBLOCKING mode")
> Reported-by: Laura Abbott <[email protected]>
> Signed-off-by: Tadeusz Struk <[email protected]>
Reviewed-by: Jarkko Sakkinen <[email protected]>
Adding to the next PR.
/Jarkko
On 1/8/20 8:04 AM, Jarkko Sakkinen wrote:
> Applied but had to fix bunch of typos, missing punctaction and
> missing parentheses in the commit message. Even checkpatch.pl
> was complaining :-/
Forgot about the checkpatch.pl thing. Sorry.
--
Tadeusz
On Wed, Jan 08, 2020 at 09:47:31AM -0800, Tadeusz Struk wrote:
> On 1/8/20 8:04 AM, Jarkko Sakkinen wrote:
> > Applied but had to fix bunch of typos, missing punctaction and
> > missing parentheses in the commit message. Even checkpatch.pl
> > was complaining :-/
>
> Forgot about the checkpatch.pl thing. Sorry.
NP, just mentioning this for the future patches.
/Jarkko