| Message ID | 20170102132213.22880-3-jarkko.sakkinen@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote on 01/02/2017 08:22:08 AM: > --- a/drivers/char/tpm/tpm2-cmd.c > +++ b/drivers/char/tpm/tpm2-cmd.c > @@ -943,7 +943,9 @@ EXPORT_SYMBOL_GPL(tpm2_probe); > */ > int tpm2_auto_startup(struct tpm_chip *chip) > { > + u32 nr_commands; > int rc; > + int i; > > rc = tpm_get_timeouts(chip); > if (rc) > @@ -967,8 +969,49 @@ int tpm2_auto_startup(struct tpm_chip *chip) > } > } > > + rc = tpm2_get_tpm_pt(chip, TPM_PT_TOTAL_COMMANDS, &nr_commands, NULL); > + if (rc) > + return rc; > + > + chip->cc_attrs_tbl = devm_kzalloc(&chip->dev, 4 * nr_commands, > + GFP_KERNEL); For some reason this devm_kzalloc bombs for the vtpm proxy driver. The only reason I could come up with is that it's being called before tpm_add_char_device() has been called. Stefan ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
On Wed, 2017-01-04 at 13:04 -0500, Stefan Berger wrote: > Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote on 01/02/2017 > 08:22:08 AM: > > > --- a/drivers/char/tpm/tpm2-cmd.c > > +++ b/drivers/char/tpm/tpm2-cmd.c > > @@ -943,7 +943,9 @@ EXPORT_SYMBOL_GPL(tpm2_probe); > > */ > > int tpm2_auto_startup(struct tpm_chip *chip) > > { > > + u32 nr_commands; > > int rc; > > + int i; > > > > rc = tpm_get_timeouts(chip); > > if (rc) > > @@ -967,8 +969,49 @@ int tpm2_auto_startup(struct tpm_chip *chip) > > } > > } > > > > + rc = tpm2_get_tpm_pt(chip, TPM_PT_TOTAL_COMMANDS, &nr_commands, > NULL); > > + if (rc) > > + return rc; > > + > > + chip->cc_attrs_tbl = devm_kzalloc(&chip->dev, 4 * nr_commands, > > + GFP_KERNEL); > > For some reason this devm_kzalloc bombs for the vtpm proxy driver. > The only reason I could come up with is that it's being called before > tpm_add_char_device() has been called. No, it should be sufficient that chip->dev be initialized (which it is in tpm_chip_alloc()). What's the error you're getting? It does look like the intention was to have non-devm with tpm_chip_alloc() and devm with tpmm_chip_alloc(), but devm_kzalloc should just work regardless because it's tied to the device model. James ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
On Wed, Jan 04, 2017 at 01:04:59PM -0500, Stefan Berger wrote: > > @@ -943,7 +943,9 @@ EXPORT_SYMBOL_GPL(tpm2_probe); > > */ > > int tpm2_auto_startup(struct tpm_chip *chip) > > { > > + u32 nr_commands; > > int rc; > > + int i; > > > > rc = tpm_get_timeouts(chip); > > if (rc) > > @@ -967,8 +969,49 @@ int tpm2_auto_startup(struct tpm_chip *chip) > > } > > } > > > > + rc = tpm2_get_tpm_pt(chip, TPM_PT_TOTAL_COMMANDS, &nr_commands, > NULL); > > + if (rc) > > + return rc; > > + > > + chip->cc_attrs_tbl = devm_kzalloc(&chip->dev, 4 * nr_commands, > > + GFP_KERNEL); > For some reason this devm_kzalloc bombs for the vtpm proxy driver. The > only reason I could come up with is that it's being called before > tpm_add_char_device() has been called. It would also fail if nr_commands is wrong, and this should be one of the array safe allocation functions since nr_command is data from the TPM... Jason ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
James Bottomley <jejb@linux.vnet.ibm.com> wrote on 01/04/2017 01:19:36 PM: > From: James Bottomley <jejb@linux.vnet.ibm.com> > To: Stefan Berger/Watson/IBM@IBMUS, Jarkko Sakkinen > <jarkko.sakkinen@linux.intel.com> > Cc: linux-security-module@vger.kernel.org, tpmdd- > devel@lists.sourceforge.net, open list <linux-kernel@vger.kernel.org> > Date: 01/04/2017 01:19 PM > Subject: Re: [tpmdd-devel] [PATCH RFC 2/4] tpm: validate TPM 2.0 commands > > On Wed, 2017-01-04 at 13:04 -0500, Stefan Berger wrote: > > Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> wrote on 01/02/2017 > > 08:22:08 AM: > > > > > --- a/drivers/char/tpm/tpm2-cmd.c > > > +++ b/drivers/char/tpm/tpm2-cmd.c > > > @@ -943,7 +943,9 @@ EXPORT_SYMBOL_GPL(tpm2_probe); > > > */ > > > int tpm2_auto_startup(struct tpm_chip *chip) > > > { > > > + u32 nr_commands; > > > int rc; > > > + int i; > > > > > > rc = tpm_get_timeouts(chip); > > > if (rc) > > > @@ -967,8 +969,49 @@ int tpm2_auto_startup(struct tpm_chip *chip) > > > } > > > } > > > > > > + rc = tpm2_get_tpm_pt(chip, TPM_PT_TOTAL_COMMANDS, &nr_commands, > > NULL); > > > + if (rc) > > > + return rc; > > > + > > > + chip->cc_attrs_tbl = devm_kzalloc(&chip->dev, 4 * nr_commands, > > > + GFP_KERNEL); > > > > For some reason this devm_kzalloc bombs for the vtpm proxy driver. > > The only reason I could come up with is that it's being called before > > tpm_add_char_device() has been called. > > No, it should be sufficient that chip->dev be initialized (which it is > in tpm_chip_alloc()). What's the error you're getting? > > It does look like the intention was to have non-devm with > tpm_chip_alloc() and devm with tpmm_chip_alloc(), but devm_kzalloc > should just work regardless because it's tied to the device model. I am running a vtpm proxy test suite. Here's the error: [ 67.596172] tpm tpm1: Operation Canceled [ 67.699052] ------------[ cut here ]------------ [ 67.699811] WARNING: CPU: 12 PID: 870 at mm/page_alloc.c:3511 __alloc_pages_slowpath+0x771/0xaf0 [ 67.701198] Modules linked in: [ 67.701400] tpm_vtpm_proxy [ 67.701642] nf_conntrack_netbios_ns nf_conntrack_broadcast [ 67.702450] ip6t_rpfilter [ 67.702662] ip6t_REJECT nf_reject_ipv6 xt_conntrack ebtable_nat [ 67.703618] ebtable_broute [ 67.703784] bridge stp llc ebtable_filter [ 67.704213] ebtables [ 67.704367] ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 [ 67.705310] nf_nat_ipv6 [ 67.705523] ip6table_mangle ip6table_security ip6table_raw ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security iptable_raw nfsd auth_rpcgss nfs_acl lockd crc32c_intel tpm_tis virtio_balloon i2c_piix4 tpm_tis_core [ 67.711414] i2c_core [ 67.711610] joydev tpm pcspkr grace sunrpc [ 67.712170] 8139too [ 67.712360] virtio_pci 8139cp virtio_ring serio_raw [ 67.713504] ata_generic [ 67.713706] mii floppy pata_acpi virtio [ 67.714891] CPU: 12 PID: 870 Comm: kworker/12:2 Not tainted 4.9.0-rc5+ #652 [ 67.715054] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.10.1-0-g8891697-prebuilt.qemu-project.org 04/01/2014 [ 67.715054] Workqueue: tpm-vtpm vtpm_proxy_work [tpm_vtpm_proxy] [ 67.715054] ffffc90002b6fa80 ffffffff8140cad1 [ 67.715054] 0000000000000000 [ 67.715054] 0000000000000000 [ 67.715054] ffffc90002b6fac0 ffffffff810a8b6b 00000db7aba7d298 00000000026000c0 [ 67.715054] 0000000000000000 0000000000000014 000000000260c0c0 ffff8802aba7ca00 [ 67.715054] Call Trace: [ 67.715054] [<ffffffff8140cad1>] dump_stack+0x63/0x82 [ 67.715054] [<ffffffff810a8b6b>] __warn+0xcb/0xf0 [ 67.715054] [<ffffffff810a8c9d>] warn_slowpath_null+0x1d/0x20 [ 67.715054] [<ffffffff811da6f1>] __alloc_pages_slowpath+0x771/0xaf0 [ 67.715054] [<ffffffff811d95e6>] ? get_page_from_freelist+0x526/0xaf0 [ 67.715054] [<ffffffff8179e583>] ? __mutex_unlock_slowpath+0xe3/0x1a0 [ 67.715054] [<ffffffff811dad9f>] __alloc_pages_nodemask+0x32f/0x390 [ 67.715054] [<ffffffff8123a4fe>] kmalloc_large_node+0x7e/0xe0 [ 67.715054] [<ffffffff81241885>] __kmalloc_node_track_caller+0x225/0x2c0 [ 67.715054] [<ffffffffa00c0f42>] ? tpm2_auto_startup+0xa2/0x2e0 [tpm] [ 67.715054] [<ffffffff815572b7>] devm_kmalloc+0x27/0x70 [ 67.715054] [<ffffffffa00c0f42>] tpm2_auto_startup+0xa2/0x2e0 [tpm] [ 67.715054] [<ffffffffa00bf3bc>] tpm_chip_register+0x5c/0x200 [tpm] [ 67.715054] [<ffffffffa029c309>] vtpm_proxy_work+0x19/0x40 [tpm_vtpm_proxy] [ 67.715054] [<ffffffff810c4593>] process_one_work+0x1f3/0x560 [ 67.715054] [<ffffffff810c4511>] ? process_one_work+0x171/0x560 [ 67.715054] [<ffffffff810c494e>] worker_thread+0x4e/0x480 [ 67.715054] [<ffffffff810c4900>] ? process_one_work+0x560/0x560 [ 67.715054] [<ffffffff810c4900>] ? process_one_work+0x560/0x560 [ 67.715054] [<ffffffff810ca994>] kthread+0xf4/0x110 [ 67.715054] [<ffffffff810ca8a0>] ? kthread_park+0x60/0x60 [ 67.715054] [<ffffffff817a1c15>] ret_from_fork+0x25/0x30 [ 67.746343] ---[ end trace 4d9abf66365987bd ]--- ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
On Wed, 2017-01-04 at 13:59 -0500, Stefan Berger wrote: > [ 67.699811] WARNING: CPU: 12 PID: 870 at mm/page_alloc.c:3511 What's the code context around this line in your source? Or what kernel version? If it's this if (order >= MAX_ORDER) { WARN_ON_ONCE(!(gfp_mask & __GFP_NOWARN)); return NULL; } Then I think you may have returned bogus data to TPM_PT_TOTAL_COMMANDS; perhaps print nr_commands. James > __alloc_pages_slowpath+0x771/0xaf0 > [ 67.701198] Modules linked in: > [ 67.701400] tpm_vtpm_proxy > [ 67.701642] nf_conntrack_netbios_ns nf_conntrack_broadcast > [ 67.702450] ip6t_rpfilter > [ 67.702662] ip6t_REJECT nf_reject_ipv6 xt_conntrack ebtable_nat > [ 67.703618] ebtable_broute > [ 67.703784] bridge stp llc ebtable_filter > [ 67.704213] ebtables > [ 67.704367] ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 > [ 67.705310] nf_nat_ipv6 > [ 67.705523] ip6table_mangle ip6table_security ip6table_raw > ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 > nf_defrag_ipv4 > nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security > iptable_raw nfsd auth_rpcgss nfs_acl lockd crc32c_intel tpm_tis > virtio_balloon i2c_piix4 tpm_tis_core > [ 67.711414] i2c_core > [ 67.711610] joydev tpm pcspkr grace sunrpc > [ 67.712170] 8139too > [ 67.712360] virtio_pci 8139cp virtio_ring serio_raw > [ 67.713504] ata_generic > [ 67.713706] mii floppy pata_acpi virtio > [ 67.714891] CPU: 12 PID: 870 Comm: kworker/12:2 Not tainted 4.9.0 > -rc5+ > #652 > [ 67.715054] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), > BIOS > rel-1.10.1-0-g8891697-prebuilt.qemu-project.org 04/01/2014 > [ 67.715054] Workqueue: tpm-vtpm vtpm_proxy_work [tpm_vtpm_proxy] > [ 67.715054] ffffc90002b6fa80 ffffffff8140cad1 > [ 67.715054] 0000000000000000 > [ 67.715054] 0000000000000000 > [ 67.715054] ffffc90002b6fac0 ffffffff810a8b6b 00000db7aba7d298 > 00000000026000c0 > [ 67.715054] 0000000000000000 0000000000000014 000000000260c0c0 > ffff8802aba7ca00 > [ 67.715054] Call Trace: > [ 67.715054] [<ffffffff8140cad1>] dump_stack+0x63/0x82 > [ 67.715054] [<ffffffff810a8b6b>] __warn+0xcb/0xf0 > [ 67.715054] [<ffffffff810a8c9d>] warn_slowpath_null+0x1d/0x20 > [ 67.715054] [<ffffffff811da6f1>] > __alloc_pages_slowpath+0x771/0xaf0 > [ 67.715054] [<ffffffff811d95e6>] ? > get_page_from_freelist+0x526/0xaf0 > [ 67.715054] [<ffffffff8179e583>] ? > __mutex_unlock_slowpath+0xe3/0x1a0 > [ 67.715054] [<ffffffff811dad9f>] > __alloc_pages_nodemask+0x32f/0x390 > [ 67.715054] [<ffffffff8123a4fe>] kmalloc_large_node+0x7e/0xe0 > [ 67.715054] [<ffffffff81241885>] > __kmalloc_node_track_caller+0x225/0x2c0 > [ 67.715054] [<ffffffffa00c0f42>] ? tpm2_auto_startup+0xa2/0x2e0 > [tpm] > [ 67.715054] [<ffffffff815572b7>] devm_kmalloc+0x27/0x70 > [ 67.715054] [<ffffffffa00c0f42>] tpm2_auto_startup+0xa2/0x2e0 > [tpm] > [ 67.715054] [<ffffffffa00bf3bc>] tpm_chip_register+0x5c/0x200 > [tpm] > [ 67.715054] [<ffffffffa029c309>] vtpm_proxy_work+0x19/0x40 > [tpm_vtpm_proxy] > [ 67.715054] [<ffffffff810c4593>] process_one_work+0x1f3/0x560 > [ 67.715054] [<ffffffff810c4511>] ? process_one_work+0x171/0x560 > [ 67.715054] [<ffffffff810c494e>] worker_thread+0x4e/0x480 > [ 67.715054] [<ffffffff810c4900>] ? process_one_work+0x560/0x560 > [ 67.715054] [<ffffffff810c4900>] ? process_one_work+0x560/0x560 > [ 67.715054] [<ffffffff810ca994>] kthread+0xf4/0x110 > [ 67.715054] [<ffffffff810ca8a0>] ? kthread_park+0x60/0x60 > [ 67.715054] [<ffffffff817a1c15>] ret_from_fork+0x25/0x30 > [ 67.746343] ---[ end trace 4d9abf66365987bd ]--- > > > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
James Bottomley <jejb@linux.vnet.ibm.com> wrote on 01/04/2017 02:05:35 PM: > From: James Bottomley <jejb@linux.vnet.ibm.com> > To: Stefan Berger/Watson/IBM@IBMUS > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>, tpmdd- > devel@lists.sourceforge.net, Jason Gunthorpe <jgunthorpe@obsidianresearch.com> > Date: 01/04/2017 02:05 PM > Subject: Re: [tpmdd-devel] [PATCH RFC 2/4] tpm: validate TPM 2.0 commands > > On Wed, 2017-01-04 at 13:59 -0500, Stefan Berger wrote: > > [ 67.699811] WARNING: CPU: 12 PID: 870 at mm/page_alloc.c:3511 > > What's the code context around this line in your source? Or what > kernel version? If it's this > > if (order >= MAX_ORDER) { > WARN_ON_ONCE(!(gfp_mask & __GFP_NOWARN)); > return NULL; > } > I am running Jarkko's tree, the tabrm branch. 4.9.0-rc5 I think. I have exactly what you are showing above. > Then I think you may have returned bogus data to TPM_PT_TOTAL_COMMANDS; > perhaps print nr_commands. Ha, what is likely the cause here is that the test suite, which implements only a few commands to respond to the kernel with from the vtpm proxy side, isn't feeding good data to the driver and the nr_commands ends up being 0... or actually bogus data / not initialized. I guess the function should check for valid input. Stefan > > James > > > __alloc_pages_slowpath+0x771/0xaf0 > > [ 67.701198] Modules linked in: > > [ 67.701400] tpm_vtpm_proxy > > [ 67.701642] nf_conntrack_netbios_ns nf_conntrack_broadcast > > [ 67.702450] ip6t_rpfilter > > [ 67.702662] ip6t_REJECT nf_reject_ipv6 xt_conntrack ebtable_nat > > [ 67.703618] ebtable_broute > > [ 67.703784] bridge stp llc ebtable_filter > > [ 67.704213] ebtables > > [ 67.704367] ip6table_nat nf_conntrack_ipv6 nf_defrag_ipv6 > > [ 67.705310] nf_nat_ipv6 > > [ 67.705523] ip6table_mangle ip6table_security ip6table_raw > > ip6table_filter ip6_tables iptable_nat nf_conntrack_ipv4 > > nf_defrag_ipv4 > > nf_nat_ipv4 nf_nat nf_conntrack iptable_mangle iptable_security > > iptable_raw nfsd auth_rpcgss nfs_acl lockd crc32c_intel tpm_tis > > virtio_balloon i2c_piix4 tpm_tis_core > > [ 67.711414] i2c_core > > [ 67.711610] joydev tpm pcspkr grace sunrpc > > [ 67.712170] 8139too > > [ 67.712360] virtio_pci 8139cp virtio_ring serio_raw > > [ 67.713504] ata_generic > > [ 67.713706] mii floppy pata_acpi virtio > > [ 67.714891] CPU: 12 PID: 870 Comm: kworker/12:2 Not tainted 4.9.0 > > -rc5+ > > #652 > > [ 67.715054] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), > > BIOS > > rel-1.10.1-0-g8891697-prebuilt.qemu-project.org 04/01/2014 > > [ 67.715054] Workqueue: tpm-vtpm vtpm_proxy_work [tpm_vtpm_proxy] > > [ 67.715054] ffffc90002b6fa80 ffffffff8140cad1 > > [ 67.715054] 0000000000000000 > > [ 67.715054] 0000000000000000 > > [ 67.715054] ffffc90002b6fac0 ffffffff810a8b6b 00000db7aba7d298 > > 00000000026000c0 > > [ 67.715054] 0000000000000000 0000000000000014 000000000260c0c0 > > ffff8802aba7ca00 > > [ 67.715054] Call Trace: > > [ 67.715054] [<ffffffff8140cad1>] dump_stack+0x63/0x82 > > [ 67.715054] [<ffffffff810a8b6b>] __warn+0xcb/0xf0 > > [ 67.715054] [<ffffffff810a8c9d>] warn_slowpath_null+0x1d/0x20 > > [ 67.715054] [<ffffffff811da6f1>] > > __alloc_pages_slowpath+0x771/0xaf0 > > [ 67.715054] [<ffffffff811d95e6>] ? > > get_page_from_freelist+0x526/0xaf0 > > [ 67.715054] [<ffffffff8179e583>] ? > > __mutex_unlock_slowpath+0xe3/0x1a0 > > [ 67.715054] [<ffffffff811dad9f>] > > __alloc_pages_nodemask+0x32f/0x390 > > [ 67.715054] [<ffffffff8123a4fe>] kmalloc_large_node+0x7e/0xe0 > > [ 67.715054] [<ffffffff81241885>] > > __kmalloc_node_track_caller+0x225/0x2c0 > > [ 67.715054] [<ffffffffa00c0f42>] ? tpm2_auto_startup+0xa2/0x2e0 > > [tpm] > > [ 67.715054] [<ffffffff815572b7>] devm_kmalloc+0x27/0x70 > > [ 67.715054] [<ffffffffa00c0f42>] tpm2_auto_startup+0xa2/0x2e0 > > [tpm] > > [ 67.715054] [<ffffffffa00bf3bc>] tpm_chip_register+0x5c/0x200 > > [tpm] > > [ 67.715054] [<ffffffffa029c309>] vtpm_proxy_work+0x19/0x40 > > [tpm_vtpm_proxy] > > [ 67.715054] [<ffffffff810c4593>] process_one_work+0x1f3/0x560 > > [ 67.715054] [<ffffffff810c4511>] ? process_one_work+0x171/0x560 > > [ 67.715054] [<ffffffff810c494e>] worker_thread+0x4e/0x480 > > [ 67.715054] [<ffffffff810c4900>] ? process_one_work+0x560/0x560 > > [ 67.715054] [<ffffffff810c4900>] ? process_one_work+0x560/0x560 > > [ 67.715054] [<ffffffff810ca994>] kthread+0xf4/0x110 > > [ 67.715054] [<ffffffff810ca8a0>] ? kthread_park+0x60/0x60 > > [ 67.715054] [<ffffffff817a1c15>] ret_from_fork+0x25/0x30 > > [ 67.746343] ---[ end trace 4d9abf66365987bd ]--- > > > > > > > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
On Wed, Jan 04, 2017 at 02:22:45PM -0500, Stefan Berger wrote: > James Bottomley <jejb@linux.vnet.ibm.com> wrote on 01/04/2017 02:05:35 PM: > > > From: James Bottomley <jejb@linux.vnet.ibm.com> > > To: Stefan Berger/Watson/IBM@IBMUS > > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>, tpmdd- > > devel@lists.sourceforge.net, Jason Gunthorpe > <jgunthorpe@obsidianresearch.com> > > Date: 01/04/2017 02:05 PM > > Subject: Re: [tpmdd-devel] [PATCH RFC 2/4] tpm: validate TPM 2.0 > commands > > > > On Wed, 2017-01-04 at 13:59 -0500, Stefan Berger wrote: > > > [ 67.699811] WARNING: CPU: 12 PID: 870 at mm/page_alloc.c:3511 > > > > What's the code context around this line in your source? Or what > > kernel version? If it's this > > > > if (order >= MAX_ORDER) { > > WARN_ON_ONCE(!(gfp_mask & __GFP_NOWARN)); > > return NULL; > > } > > > > I am running Jarkko's tree, the tabrm branch. 4.9.0-rc5 I think. I have > exactly what you are showing above. > > Then I think you may have returned bogus data to TPM_PT_TOTAL_COMMANDS; > > perhaps print nr_commands. > > Ha, what is likely the cause here is that the test suite, which implements > only a few commands to respond to the kernel with from the vtpm proxy > side, isn't feeding good data to the driver and the nr_commands ends up > being 0... or actually bogus data / not initialized. I guess the function > should check for valid input. So, what kind of validation do you suggest? Checking it whether it is zero? /Jarkko ------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi
On 01/09/2017 05:17 PM, Jarkko Sakkinen wrote: > On Wed, Jan 04, 2017 at 02:22:45PM -0500, Stefan Berger wrote: >> James Bottomley <jejb@linux.vnet.ibm.com> wrote on 01/04/2017 02:05:35 PM: >> >> > From: James Bottomley <jejb@linux.vnet.ibm.com> >> > To: Stefan Berger/Watson/IBM@IBMUS >> > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>, tpmdd- >> > devel@lists.sourceforge.net, Jason Gunthorpe >> <jgunthorpe@obsidianresearch.com> >> > Date: 01/04/2017 02:05 PM >> > Subject: Re: [tpmdd-devel] [PATCH RFC 2/4] tpm: validate TPM 2.0 >> commands >> > >> > On Wed, 2017-01-04 at 13:59 -0500, Stefan Berger wrote: >> > > [ 67.699811] WARNING: CPU: 12 PID: 870 at mm/page_alloc.c:3511 >> > >> > What's the code context around this line in your source? Or what >> > kernel version? If it's this >> > >> > if (order >= MAX_ORDER) { >> > WARN_ON_ONCE(!(gfp_mask & __GFP_NOWARN)); >> > return NULL; >> > } >> > >> >> I am running Jarkko's tree, the tabrm branch. 4.9.0-rc5 I think. I have >> exactly what you are showing above. >> > Then I think you may have returned bogus data to TPM_PT_TOTAL_COMMANDS; >> > perhaps print nr_commands. >> >> Ha, what is likely the cause here is that the test suite, which implements >> only a few commands to respond to the kernel with from the vtpm proxy >> side, isn't feeding good data to the driver and the nr_commands ends up >> being 0... or actually bogus data / not initialized. I guess the function >> should check for valid input. > So, what kind of validation do you suggest? Checking it whether it is > zero? Out of bounds. > 0x200, < 0x01 Stefan ------------------------------------------------------------------------------ Developer Access Program for Intel Xeon Phi Processors Access to Intel Xeon Phi processor-based developer platforms. With one year of Intel Parallel Studio XE. Training and support from Colfax. Order your platform today. http://sdm.link/xeonphi
diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c index 769d8b0..0794a5d3 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -328,6 +328,36 @@ unsigned long tpm_calc_ordinal_duration(struct tpm_chip *chip, } EXPORT_SYMBOL_GPL(tpm_calc_ordinal_duration); +static bool tpm_validate_command(struct tpm_chip *chip, const u8 *cmd, + size_t len) +{ + const struct tpm_input_header *header = (const void *)cmd; + u32 cc; + size_t len_min = TPM_HEADER_SIZE; + u32 attrs; + + if ((len >= len_min) && (chip->flags & TPM_CHIP_FLAG_TPM2) && + chip->nr_commands) { + cc = be32_to_cpu(header->ordinal); + if (!tpm2_find_cc_attrs(chip, cc, &attrs)) { + dev_dbg(&chip->dev, "0x%04x is an invalid command\n", + cc); + return false; + } + len_min += + 4 * ((attrs >> TPM2_CC_ATTR_CHANDLES) & GENMASK(2, 0)); + } + + if (len < len_min) { + dev_dbg(&chip->dev, + "%s: insufficient command length %zu < %zu\n", + __func__, len, len_min); + return false; + } + + return true; +} + /** * tmp_transmit - Internal kernel interface to transmit TPM commands. * @@ -347,7 +377,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz, u32 count, ordinal; unsigned long stop; - if (bufsiz < TPM_HEADER_SIZE) + if (!tpm_validate_command(chip, buf, bufsiz)) return -EINVAL; if (bufsiz > TPM_BUFSIZE) diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index c74a663..ed21c2c 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -127,7 +127,12 @@ enum tpm2_permanent_handles { }; enum tpm2_capabilities { - TPM2_CAP_TPM_PROPERTIES = 6, + TPM2_CAP_COMMANDS = 2, + TPM2_CAP_TPM_PROPERTIES = 6, +}; + +enum tpm2_properties { + TPM_PT_TOTAL_COMMANDS = 0x0129, }; enum tpm2_startup_types { @@ -135,6 +140,11 @@ enum tpm2_startup_types { TPM2_SU_STATE = 0x0001, }; +enum tpm2_cc_attrs { + TPM2_CC_ATTR_CHANDLES = 25, + TPM2_CC_ATTR_RHANDLE = 28, +}; + #define TPM_VID_INTEL 0x8086 #define TPM_VID_WINBOND 0x1050 #define TPM_VID_STM 0x104A @@ -207,6 +217,8 @@ struct tpm_chip { #endif /* CONFIG_ACPI */ struct tpm_buf tr_buf; + u32 nr_commands; + u32 *cc_attrs_tbl; }; #define to_tpm_chip(d) container_of(d, struct tpm_chip, dev) @@ -534,4 +546,5 @@ int tpm2_auto_startup(struct tpm_chip *chip); void tpm2_shutdown(struct tpm_chip *chip, u16 shutdown_type); unsigned long tpm2_calc_ordinal_duration(struct tpm_chip *chip, u32 ordinal); int tpm2_probe(struct tpm_chip *chip); +bool tpm2_find_cc_attrs(struct tpm_chip *chip, u32 cc, u32 *attrs); #endif diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c index f0e0807..fa928c7 100644 --- a/drivers/char/tpm/tpm2-cmd.c +++ b/drivers/char/tpm/tpm2-cmd.c @@ -943,7 +943,9 @@ EXPORT_SYMBOL_GPL(tpm2_probe); */ int tpm2_auto_startup(struct tpm_chip *chip) { + u32 nr_commands; int rc; + int i; rc = tpm_get_timeouts(chip); if (rc) @@ -967,8 +969,49 @@ int tpm2_auto_startup(struct tpm_chip *chip) } } + rc = tpm2_get_tpm_pt(chip, TPM_PT_TOTAL_COMMANDS, &nr_commands, NULL); + if (rc) + return rc; + + chip->cc_attrs_tbl = devm_kzalloc(&chip->dev, 4 * nr_commands, + GFP_KERNEL); + + tpm_buf_init(&chip->tr_buf, TPM2_ST_NO_SESSIONS, + TPM2_CC_GET_CAPABILITY); + tpm_buf_append_u32(&chip->tr_buf, TPM2_CAP_COMMANDS); + tpm_buf_append_u32(&chip->tr_buf, TPM2_CC_FIRST); + tpm_buf_append_u32(&chip->tr_buf, nr_commands); + + rc = tpm_transmit_cmd(chip, chip->tr_buf.data, TPM_BUFSIZE, 0, NULL); + if (rc < 0) + goto out; + + if (nr_commands != + be32_to_cpup((__be32 *)&chip->tr_buf.data[TPM_HEADER_SIZE + 5])) + return -EINVAL; + + for (i = 0; i < nr_commands; i++) + chip->cc_attrs_tbl[i] = be32_to_cpup( + (u32 *)&chip->tr_buf.data[TPM_HEADER_SIZE + 9 + 4 * i]); + + chip->nr_commands = nr_commands; + out: if (rc > 0) rc = -ENODEV; return rc; } + +bool tpm2_find_cc_attrs(struct tpm_chip *chip, u32 cc, u32 *attrs) +{ + int i; + + for (i = 0; i < chip->nr_commands; i++) { + if (cc == (chip->cc_attrs_tbl[i] & GENMASK(15, 0))) { + *attrs = chip->cc_attrs_tbl[i]; + return true; + } + } + + return false; +}
Check for every TPM 2.0 command that the command code is supported and the command buffer has at least the length that can contain the header and the handle area. Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> --- drivers/char/tpm/tpm-interface.c | 32 +++++++++++++++++++++++++++++- drivers/char/tpm/tpm.h | 15 +++++++++++++- drivers/char/tpm/tpm2-cmd.c | 43 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 88 insertions(+), 2 deletions(-)