| Message ID | 20240521154028.3339742-1-mochs@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | tpm_tis_spi: Account for SPI header when allocating TPM SPI xfer buffer | expand |
On Tue May 21, 2024 at 6:40 PM EEST, Matthew R. Ochs wrote: > The TPM SPI transfer mechanism uses MAX_SPI_FRAMESIZE for computing the > maximum transfer length and the size of the transfer buffer. As such, it > does not account for the 4 bytes of header that prepends the SPI data > frame. This can result in out-of-bounds accesses and was confirmed with > KASAN. > > Introduce MAX_SPI_BUFSIZE to account for the header and use to allocate > the transfer buffer. > > Fixes: a86a42ac2bd6 ("tpm_tis_spi: Add hardware wait polling") > Signed-off-by: Matthew R. Ochs <mochs@nvidia.com> > Tested-by: Carol Soto <csoto@nvidia.com> > --- > drivers/char/tpm/tpm_tis_spi_main.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c > index 3f9eaf27b41b..ba50eaead9d8 100644 > --- a/drivers/char/tpm/tpm_tis_spi_main.c > +++ b/drivers/char/tpm/tpm_tis_spi_main.c > @@ -37,6 +37,8 @@ > #include "tpm_tis_spi.h" > > #define MAX_SPI_FRAMESIZE 64 > +#define MAX_SPI_HDRSIZE 4 > +#define MAX_SPI_BUFSIZE (MAX_SPI_HDRSIZE + MAX_SPI_FRAMESIZE) "MAX_" prefix does not make sense in the new entries. > > /* > * TCG SPI flow control is documented in section 6.4 of the spec[1]. In short, > @@ -247,7 +249,7 @@ static int tpm_tis_spi_write_bytes(struct tpm_tis_data *data, u32 addr, > int tpm_tis_spi_init(struct spi_device *spi, struct tpm_tis_spi_phy *phy, > int irq, const struct tpm_tis_phy_ops *phy_ops) > { > - phy->iobuf = devm_kmalloc(&spi->dev, MAX_SPI_FRAMESIZE, GFP_KERNEL); > + phy->iobuf = devm_kmalloc(&spi->dev, MAX_SPI_BUFSIZE, GFP_KERNEL); It would better to open code here "SPI_HDRSIZE + MAX_SPI_FRAMESIZE". I.e. less cross-referencing and documents better what is going on at the call site. > if (!phy->iobuf) > return -ENOMEM; > BR, Jarkko
> On May 21, 2024, at 10:55 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote: >> >> /* >> * TCG SPI flow control is documented in section 6.4 of the spec[1]. In short, >> @@ -247,7 +249,7 @@ static int tpm_tis_spi_write_bytes(struct tpm_tis_data *data, u32 addr, >> int tpm_tis_spi_init(struct spi_device *spi, struct tpm_tis_spi_phy *phy, >> int irq, const struct tpm_tis_phy_ops *phy_ops) >> { >> - phy->iobuf = devm_kmalloc(&spi->dev, MAX_SPI_FRAMESIZE, GFP_KERNEL); >> + phy->iobuf = devm_kmalloc(&spi->dev, MAX_SPI_BUFSIZE, GFP_KERNEL); > > It would better to open code here "SPI_HDRSIZE + MAX_SPI_FRAMESIZE". > > I.e. less cross-referencing and documents better what is going on at > the call site. Sure, will make this change in a v2. -matt
On Tue May 21, 2024 at 8:59 PM EEST, Matt Ochs wrote: > > On May 21, 2024, at 10:55 AM, Jarkko Sakkinen <jarkko@kernel.org> wrote: > >> > >> /* > >> * TCG SPI flow control is documented in section 6.4 of the spec[1]. In short, > >> @@ -247,7 +249,7 @@ static int tpm_tis_spi_write_bytes(struct tpm_tis_data *data, u32 addr, > >> int tpm_tis_spi_init(struct spi_device *spi, struct tpm_tis_spi_phy *phy, > >> int irq, const struct tpm_tis_phy_ops *phy_ops) > >> { > >> - phy->iobuf = devm_kmalloc(&spi->dev, MAX_SPI_FRAMESIZE, GFP_KERNEL); > >> + phy->iobuf = devm_kmalloc(&spi->dev, MAX_SPI_BUFSIZE, GFP_KERNEL); > > > > It would better to open code here "SPI_HDRSIZE + MAX_SPI_FRAMESIZE". > > > > I.e. less cross-referencing and documents better what is going on at > > the call site. > > Sure, will make this change in a v2. Yeah, and thanks for spotting the bug and fixing it! Looking forward to the final fix. BR, Jarkko
diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c index 3f9eaf27b41b..ba50eaead9d8 100644 --- a/drivers/char/tpm/tpm_tis_spi_main.c +++ b/drivers/char/tpm/tpm_tis_spi_main.c @@ -37,6 +37,8 @@ #include "tpm_tis_spi.h" #define MAX_SPI_FRAMESIZE 64 +#define MAX_SPI_HDRSIZE 4 +#define MAX_SPI_BUFSIZE (MAX_SPI_HDRSIZE + MAX_SPI_FRAMESIZE) /* * TCG SPI flow control is documented in section 6.4 of the spec[1]. In short, @@ -247,7 +249,7 @@ static int tpm_tis_spi_write_bytes(struct tpm_tis_data *data, u32 addr, int tpm_tis_spi_init(struct spi_device *spi, struct tpm_tis_spi_phy *phy, int irq, const struct tpm_tis_phy_ops *phy_ops) { - phy->iobuf = devm_kmalloc(&spi->dev, MAX_SPI_FRAMESIZE, GFP_KERNEL); + phy->iobuf = devm_kmalloc(&spi->dev, MAX_SPI_BUFSIZE, GFP_KERNEL); if (!phy->iobuf) return -ENOMEM;
The TPM SPI transfer mechanism uses MAX_SPI_FRAMESIZE for computing the maximum transfer length and the size of the transfer buffer. As such, it does not account for the 4 bytes of header that prepends the SPI data frame. This can result in out-of-bounds accesses and was confirmed with KASAN. Introduce MAX_SPI_BUFSIZE to account for the header and use to allocate the transfer buffer. Fixes: a86a42ac2bd6 ("tpm_tis_spi: Add hardware wait polling") Signed-off-by: Matthew R. Ochs <mochs@nvidia.com> Tested-by: Carol Soto <csoto@nvidia.com> --- drivers/char/tpm/tpm_tis_spi_main.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)