Bad usercopy from tpm after d23d12484307 ("tpm: fix invalid locking in NONBLOCKING mode")

Bad usercopy from tpm after d23d12484307 ("tpm: fix invalid locking in NONBLOCKING mode")

[Laura Abbott]

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

Re: Bad usercopy from tpm after d23d12484307 ("tpm: fix invalid locking in NONBLOCKING mode")

[Tadeusz Struk]

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

[PATCH] tpm: handle negative priv->response_len in tpm_common_read

[Tadeusz Struk]

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: stable@vger.kernel.org
Fixes: d23d12484307 ("tpm: fix invalid locking in NONBLOCKING mode")
Reported-by: Laura Abbott <labbott@redhat.com>
Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
---
 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;
 

Re: [PATCH] tpm: handle negative priv->response_len in tpm_common_read

[Jarkko Sakkinen]

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: stable@vger.kernel.org
> Fixes: d23d12484307 ("tpm: fix invalid locking in NONBLOCKING mode")
> Reported-by: Laura Abbott <labbott@redhat.com>
> Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

Adding to the next PR.

/Jarkko

Re: [PATCH] tpm: handle negative priv->response_len in tpm_common_read

[Jarkko Sakkinen]

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: stable@vger.kernel.org
> > Fixes: d23d12484307 ("tpm: fix invalid locking in NONBLOCKING mode")
> > Reported-by: Laura Abbott <labbott@redhat.com>
> > Signed-off-by: Tadeusz Struk <tadeusz.struk@intel.com>
> 
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> 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

Re: [PATCH] tpm: handle negative priv->response_len in tpm_common_read

[Tadeusz Struk]

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

Re: [PATCH] tpm: handle negative priv->response_len in tpm_common_read

[Jarkko Sakkinen]

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