Re: [PATCH v10, RESEND 4/6] tpm: move tpm_chip definition to include/linux/tpm.h

[Nathan Chancellor]

On Fri, Feb 08, 2019 at 09:41:14AM +0100, Roberto Sassu wrote:
> On 2/8/2019 5:24 AM, Nathan Chancellor wrote:
> > > diff --git a/include/linux/tpm.h b/include/linux/tpm.h
> > > index afd022fc9d3d..816e686a73ac 100644
> > > --- a/include/linux/tpm.h
> > > +++ b/include/linux/tpm.h
> > > @@ -22,6 +22,10 @@
> > >   #ifndef __LINUX_TPM_H__
> > >   #define __LINUX_TPM_H__
> > > +#include <linux/hw_random.h>
> > > +#include <linux/acpi.h>
> 
> Hi Nathan
> 
> I think the error comes from the line above.
> security/integrity/ima/ima.h includes <linux/tpm.h>, which now includes
> <linux/acpi.h>, which includes <asm/acpi.h>, which includes
> <linux/efi.h> (for the arm64 architecture only). Both ima.h and efi.h
> define 'NONE'.
> 

Thank you for providing that analysis, I appreciate it!

> The solution would be to rename one of them. I'm not familiar with the
> EFI part. Renaming 'NONE' in IMA should be easy as it is not used
> anywhere.
> 

This seems reasonable, no?

Thanks,
Nathan

========================================================================

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index d213e835c498..f203a86f1f23 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -173,7 +173,7 @@ static inline unsigned long ima_hash_key(u8 *digest)
 }
 
 #define __ima_hooks(hook)              \
-       hook(NONE)                      \
+       hook(NO_CHECK)                  \
        hook(FILE_CHECK)                \
        hook(MMAP_CHECK)                \
        hook(BPRM_CHECK)                \

> Thanks
> 
> Roberto
> 
> 
> > > +#include <linux/cdev.h>
> > > +#include <linux/fs.h>
> > >   #include <crypto/hash_info.h>
> > >   #define TPM_DIGEST_SIZE 20	/* Max TPM v1.2 PCR size */
> > > @@ -75,6 +79,93 @@ struct tpm_class_ops {
> > >   	void (*clk_enable)(struct tpm_chip *chip, bool value);
> > >   };
> > > +#define TPM_NUM_EVENT_LOG_FILES		3
> > > +
> > > +/* Indexes the duration array */
> > > +enum tpm_duration {
> > > +	TPM_SHORT = 0,
> > > +	TPM_MEDIUM = 1,
> > > +	TPM_LONG = 2,
> > > +	TPM_LONG_LONG = 3,
> > > +	TPM_UNDEFINED,
> > > +	TPM_NUM_DURATIONS = TPM_UNDEFINED,
> > > +};
> > > +
> > > +#define TPM_PPI_VERSION_LEN		3
> > > +
> > > +struct tpm_space {
> > > +	u32 context_tbl[3];
> > > +	u8 *context_buf;
> > > +	u32 session_tbl[3];
> > > +	u8 *session_buf;
> > > +};
> > > +
> > > +struct tpm_bios_log {
> > > +	void *bios_event_log;
> > > +	void *bios_event_log_end;
> > > +};
> > > +
> > > +struct tpm_chip_seqops {
> > > +	struct tpm_chip *chip;
> > > +	const struct seq_operations *seqops;
> > > +};
> > > +
> > > +struct tpm_chip {
> > > +	struct device dev;
> > > +	struct device devs;
> > > +	struct cdev cdev;
> > > +	struct cdev cdevs;
> > > +
> > > +	/* A driver callback under ops cannot be run unless ops_sem is held
> > > +	 * (sometimes implicitly, eg for the sysfs code). ops becomes null
> > > +	 * when the driver is unregistered, see tpm_try_get_ops.
> > > +	 */
> > > +	struct rw_semaphore ops_sem;
> > > +	const struct tpm_class_ops *ops;
> > > +
> > > +	struct tpm_bios_log log;
> > > +	struct tpm_chip_seqops bin_log_seqops;
> > > +	struct tpm_chip_seqops ascii_log_seqops;
> > > +
> > > +	unsigned int flags;
> > > +
> > > +	int dev_num;		/* /dev/tpm# */
> > > +	unsigned long is_open;	/* only one allowed */
> > > +
> > > +	char hwrng_name[64];
> > > +	struct hwrng hwrng;
> > > +
> > > +	struct mutex tpm_mutex;	/* tpm is processing */
> > > +
> > > +	unsigned long timeout_a; /* jiffies */
> > > +	unsigned long timeout_b; /* jiffies */
> > > +	unsigned long timeout_c; /* jiffies */
> > > +	unsigned long timeout_d; /* jiffies */
> > > +	bool timeout_adjusted;
> > > +	unsigned long duration[TPM_NUM_DURATIONS]; /* jiffies */
> > > +	bool duration_adjusted;
> > > +
> > > +	struct dentry *bios_dir[TPM_NUM_EVENT_LOG_FILES];
> > > +
> > > +	const struct attribute_group *groups[3];
> > > +	unsigned int groups_cnt;
> > > +
> > > +	u32 nr_allocated_banks;
> > > +	struct tpm_bank_info *allocated_banks;
> > > +#ifdef CONFIG_ACPI
> > > +	acpi_handle acpi_dev_handle;
> > > +	char ppi_version[TPM_PPI_VERSION_LEN + 1];
> > > +#endif /* CONFIG_ACPI */
> > > +
> > > +	struct tpm_space work_space;
> > > +	u32 last_cc;
> > > +	u32 nr_commands;
> > > +	u32 *cc_attrs_tbl;
> > > +
> > > +	/* active locality */
> > > +	int locality;
> > > +};
> > > +
> > >   #if defined(CONFIG_TCG_TPM) || defined(CONFIG_TCG_TPM_MODULE)
> > >   extern int tpm_is_tpm2(struct tpm_chip *chip);
> > > -- 
> > > 2.17.1
> > > 
> > 
> > Hi Robert,
> > 
> > This patch causes a build error with Clang (bisected on next-20190207):
> > 
> > security/integrity/ima/ima.h:191:2: error: redefinition of enumerator 'NONE'
> >          __ima_hooks(__ima_hook_enumify)
> >          ^
> > security/integrity/ima/ima.h:176:7: note: expanded from macro '__ima_hooks'
> >          hook(NONE)                      \
> >               ^
> > include/linux/efi.h:1709:2: note: previous definition is here
> >          NONE,
> >          ^
> > 1 error generated.
> > 
> > I am not sure how to reconcile this otherwise I would have sent a patch.
> > 
> > Thanks,
> > Nathan
> > 
> 
> -- 
> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> Managing Director: Bo PENG, Jian LI, Yanli SHI