Hi Jason,
Thanks for review, Please find my responses inline.
On 07/29/2016 10:44 PM, Jason Gunthorpe wrote:
> On Fri, Jul 29, 2016 at 02:44:39AM -0400, Nayna Jain wrote:
>
>> + chip->bios_dir = tpm_bios_log_setup(chip);
>> +
>
> And the next somewhat pre-existing issue is that we call
> tpm_bios_log_setup even if we don't have access to a bios log.
>
> Does the bios log ever change or is it static at boot? Can we move
> read_log into the chip and do it once so we can tell if it worked
> before creating security fs stuff??
I am not sure right away if it is safe to move read_log into the chip,
will look into this.
>
>> +++ b/drivers/char/tpm/tpm2.h
>
> You should probably pick a different name if this is only bios log
> stuff.
My thought was to have all declarations related to TPM2 in tpm2.h Some
of them as I add support might get moved from tpm2-cmd.c to this file.
So, didn't intend to have it only for log.
I see that for TPM1.2, there are two header files, one for eventlog i.e.
tpm_eventlog.h and other for remaining stuff that is tpm.h. I wasn't
sure if really needed two files, so for tpm2, I thought can have one
header file as tpm2.h.
Please let me know if it doesn't sound ok.
>
>> +struct tcg_efispecideventalgorithmsize {
>> + u16 algorithm_id;
>> + u16 digest_size;
>> +} __packed;
>
> The bios log is defined to be host endian?
>
> Please refernce the standard in a comment that these structs are
> coming from.
Sure, I will add the reference to the standard in comment.
>
>> +int read_log(struct tpm_bios_log *log)
>> +{
>> + struct device_node *np;
>> + u32 logSize;
>> + const u32 *sizep;
>> + const u64 *basep;
>> +
>> + if (log->bios_event_log != NULL) {
>> + pr_err("%s: ERROR - Eventlog already initialized\n",
>> __func__);
>
> No to all this logging.
>
> If you really need some of it then use dev_dbg(&chip->dev) pr_err is
> not acceptable.
Sure.will update it.
>
> Yes, this means you need to safely get tpm_chip into read_log. Please
> make that a singular patch because the lifetime model will be quite
> complex.
Sorry, I didn't get this. Can you please help me with understanding what
does singular patch and lifetime model here imply ?
>
>> + np = of_find_node_by_name(NULL, "tpm");
>> + if (!np) {
>> + pr_err("%s: ERROR - tpm entry not supported\n", __func__);
>> + return -ENODEV;
>> + }
>
> If you are doing of stuff then you need to document it in
> Documentation/device-tree, so NAK on this without a proper
> documentation patch. (make sure you follow the special rules for these
> patches)
Sure, will add this file.
>
> What is 'tpm' ? Is that the DT node that the TPM driver is binding
> too? If so then you need to use chip->pdev->of_node instead of
> 'of_find_node_by_name'.
Yes it is device tree node. Ok. Sure will change this for both tpm2_of.c
and tpm_of.c
>
> Same comments applies to the pre-existing tpm_of.c, please fix that as
> well.
Sure.
>
> Why have you substantially copied tpm_of and called it tpm2_of? Don't
> do that.
Sure, will look into this.
>
>> +
>> + sizep = of_get_property(np, "linux,sml-size", NULL);
>> + if (sizep == NULL) {
>> + pr_err("%s: ERROR - sml-get-allocated-size not found\n",
>> + __func__);
>> + goto cleanup_eio;
>> + }
>> + logSize = be32_to_cpup(sizep);
>
> If you call endianness converters then make sure to tag
> properly. eg sizep should be a be32, etc. Audit everything in
> eventlog, I saw other cases..
Sure. will update.
>
>> + log->bios_event_log = kmalloc(logSize, GFP_KERNEL);
>> + if (!log->bios_event_log) {
>> + pr_err("%s: ERROR - Not enough memory for firmware
>> measurements\n",
>> + __func__);
>
> Never log for ENOMEM, the kernel logs extensively already.
Sure, will update.
>
>> #if defined(CONFIG_TCG_IBMVTPM) || defined(CONFIG_TCG_IBMVTPM_MODULE) || \
>> - defined(CONFIG_ACPI)
>> -extern struct dentry **tpm_bios_log_setup(const char *);
>> -extern void tpm_bios_log_teardown(struct dentry **);
>> + defined(CONFIG_ACPI) || defined(CONFIG_PPC64)
>> +extern struct dentry **tpm_bios_log_setup(struct tpm_chip *chip);
>> +extern void tpm_bios_log_teardown(struct tpm_chip *chip);
>
> I'm really deeply unhappy about these ifdefs and the general scheme
> that was used to force this special difference into the tpm core.
>
> Please find another way to do it.
>
> IMHO, 'read_log' should simply try ACPI and OF in sequence to find the
> log.
Ok, Sure, will look into this.
>
>> + num_files = chip->flags & TPM_CHIP_FLAG_TPM2 ? 2 : 3;
>> + ret = kmalloc_array(num_files, sizeof(struct dentry *), GFP_KERNEL);
>> + if (!ret)
>> + goto out;
>
> Follow the sysfs pattern please.
Sure. will fix this.
I will take care of all above comments in my next version which I will post.
Thanks & Regards,
- Nayna
>
> Jason
>
------------------------------------------------------------------------------
_______________________________________________
tpmdd-devel mailing list
tpmdd-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
