linux-kernel - Re: TPM: STMicroelectronics st33 driver SPI

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 26 Aug 2013 22:28:10 -0500 (CDT)
From:	Ashley Lai <ashley@...leylai.com>
To:	Mathias.LEBLANC@...com, jean-luc.blanc@...com,
	Peter Hüwe <PeterHuewe@....de>,
	linux-kernel@...r.kernel.org
cc:	tpmdd-devel@...ts.sourceforge.net,
	Leonidas Da Silva Barbosa <leosilva@...ux.vnet.ibm.com>,
	"Rajiv Andrade (mail@...jiv.net)" <mail@...jiv.net>,
	"Sirrix AG (tpmdd@...rix.com)" <tpmdd@...rix.com>,
	andy.shevchenko@...il.com, adlai@...ux.vnet.ibm.com
Subject: Re: TPM: STMicroelectronics st33 driver SPI


Please see my comments below.  Did you get a chance to run the trousers 
testsuite?

git://trousers.git.sourceforge.net/gitroot/trousers/testsuite

+static u8 spi_read8_reg(struct tpm_chip *tpm, u8 tpm_register,
+                   u8 *tpm_data, u16 tpm_size)
+{
+       ...
+
+       /* header + status byte + size of the data + status byte */
+       value = spi_sync_transfer(dev, &xfer, 1);
+
+       if (tpm_size > 0 && value == 0) {
+               nbr_dummy_bytes = 2;
Why use 2 for the dummy bytes?  Some comments here would be 
helpful.  Should we use a #define for 2?

...

+static unsigned long wait_for_serirq_timeout(struct tpm_chip *chip,
+        bool condition, unsigned long timeout)
+{
+       long status = 0;
+       struct spi_device *client;
+       struct st33zp24_platform_data *pin_infos;
+
+       client = (struct spi_device __force *)chip->vendor.iobase;
+       pin_infos = client->dev.platform_data;
+
+       status = wait_for_completion_interruptible_timeout(
+                               &pin_infos->irq_detection, timeout);
+       if (status > 0)
+               enable_irq(gpio_to_irq(pin_infos->io_serirq));
+       gpio_direction_input(pin_infos->io_serirq);
+
+       if (!status)
+               return -EBUSY;
This function returns -EBUSY but the return function type is unsigned.

...

+static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
+{
+       u32 size = 0, burstcnt, len;
+       long status = 0;
+
+       while (size < count &&
+              wait_for_stat(chip,
+                            TPM_STS_DATA_AVAIL | TPM_STS_VALID,
+                            chip->vendor.timeout_c,
+                            &chip->vendor.read_queue)
+                                               == 0) {
+               burstcnt = get_burstcount(chip);
+               len = min_t(int, burstcnt, count - size);

What if get_burstcount returns -EBUSY?  Should burstcnt be checked
and handled here in case of error?

+               status = spi_read8_reg(chip, TPM_DATA_FIFO, buf + size, 
len);
+               if (status < 0)
+                       return status;
+
+
Extra empty line here.
+               size += len;

See above comment.  If len is -EBUSY, there might be an issue here.

+       }
+return size;

...

+static int tpm_stm_spi_send(struct tpm_chip *chip, unsigned char *buf,
+                           size_t len)
+{
...
+	for (i = 0; i < len - 1;) {
+		burstcnt = get_burstcount(chip);
Again burstcnt needs to be checked to make sure it is > 0.

+		size = min_t(int, len - i - 1, burstcnt);
+		ret = spi_write8_reg(chip, TPM_DATA_FIFO, buf, size);
+		if (ret < 0)
+			goto out_err;
+		i += size;
+	}
...
+       /* write last byte */
+       spi_write8_reg(chip, TPM_DATA_FIFO, buf + len - 1, 1);
Should we check for the return code here?  A few lines below it checks 
for the return value.
...
+
+
+       /* go and do it */
+       data = TPM_STS_GO;
+       ret = spi_write8_reg(chip, TPM_STS, &data, 1);

Regards,
--Ashley Lai


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/