[U-Boot] [PATCH 1/3] drivers/tpm/tpm_tis_sandbox.c: Fix uninitialized variable use
In rollback_space_kernel we were not initializing the reserved fields which should be for safety sake, and doing memset here means we don't need to set the version field specifically either.
Reported-by: Coverity (CID: 143917) Cc: Simon Glass sjg@chromium.org Signed-off-by: Tom Rini trini@konsulko.com --- drivers/tpm/tpm_tis_sandbox.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/tpm/tpm_tis_sandbox.c b/drivers/tpm/tpm_tis_sandbox.c index 4aade56..e7746dc 100644 --- a/drivers/tpm/tpm_tis_sandbox.c +++ b/drivers/tpm/tpm_tis_sandbox.c @@ -214,9 +214,9 @@ static int sandbox_tpm_xfer(struct udevice *dev, const uint8_t *sendbuf,
data = recvbuf + TPM_RESPONSE_HEADER_LENGTH + sizeof(uint32_t); + memset(&rsk, 0, sizeof(struct rollback_space_kernel)); rsk.struct_version = 2; rsk.uid = ROLLBACK_SPACE_KERNEL_UID; - rsk.kernel_versions = 0; rsk.crc8 = crc8(0, (unsigned char *)&rsk, offsetof(struct rollback_space_kernel, crc8));
If get_dev_addr fails it will return FDT_ADDR_T_NONE and:
"priv->pid == 4294967295U" is always false regardless of the values of its operands. This occurs as the logical operand of if.
Cc: Mateusz Kulikowski mateusz.kulikowski@gmail.com Reported-by: Coverity (CID: 143913) Signed-off-by: Tom Rini trini@konsulko.com --- drivers/gpio/pm8916_gpio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpio/pm8916_gpio.c b/drivers/gpio/pm8916_gpio.c index 1abab7f..0b61975 100644 --- a/drivers/gpio/pm8916_gpio.c +++ b/drivers/gpio/pm8916_gpio.c @@ -50,7 +50,7 @@ DECLARE_GLOBAL_DATA_PTR; #define REG_EN_CTL_ENABLE (1 << 7)
struct pm8916_gpio_bank { - uint16_t pid; /* Peripheral ID on SPMI bus */ + uint32_t pid; /* Peripheral ID on SPMI bus */ };
static int pm8916_gpio_set_direction(struct udevice *dev, unsigned offset,
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
Hi,
Sorry for slow reply.
On 12.04.2016 21:11, Tom Rini wrote:
If get_dev_addr fails it will return FDT_ADDR_T_NONE and:
"priv->pid == 4294967295U" is always false regardless of the values of its operands. This occurs as the logical operand of if.Cc: Mateusz Kulikowski mateusz.kulikowski@gmail.com Reported-by: Coverity (CID: 143913)
[...]
- uint16_t pid; /* Peripheral ID on SPMI bus */
- uint32_t pid; /* Peripheral ID on SPMI bus */
};
static int pm8916_gpio_set_direction(struct udevice *dev, unsigned offset,
Note applies to two patches in this series (pm8916_gpio.c and pm8916.c)
I think (now, when the coverity pointed out mistake) that we should add in that case check if pid fits in 16-bits, as this is maximum pid value on spmi bus.
This checks should be done in pm8916_gpio_probe() and pm8916_probe().
Would you like to do it in your series or want me to post another patch on top of them?
Regards, Mateusz
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
On 18.04.2016 22:23, Mateusz Kulikowski wrote:
Hi,
Sorry for slow reply.
On 12.04.2016 21:11, Tom Rini wrote:
If get_dev_addr fails it will return FDT_ADDR_T_NONE and:
"priv->pid == 4294967295U" is always false regardless of the values of its operands. This occurs as the logical operand of if.Cc: Mateusz Kulikowski mateusz.kulikowski@gmail.com Reported-by: Coverity (CID: 143913)
[...]
- uint16_t pid; /* Peripheral ID on SPMI bus */
- uint32_t pid; /* Peripheral ID on SPMI bus */
};
static int pm8916_gpio_set_direction(struct udevice *dev, unsigned offset,
Note applies to two patches in this series (pm8916_gpio.c and pm8916.c)
I think (now, when the coverity pointed out mistake) that we should add in that case check if pid fits in 16-bits, as this is maximum pid value on spmi bus.
This checks should be done in pm8916_gpio_probe() and pm8916_probe().
Would you like to do it in your series or want me to post another patch on top of them?
Or even better - leave it as uint16_t and do check & cast in probe()
Matuesz
On Mon, Apr 18, 2016 at 10:24:20PM +0200, Mateusz Kulikowski wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
On 18.04.2016 22:23, Mateusz Kulikowski wrote:
Hi,
Sorry for slow reply.
On 12.04.2016 21:11, Tom Rini wrote:
If get_dev_addr fails it will return FDT_ADDR_T_NONE and:
"priv->pid == 4294967295U" is always false regardless of the values of its operands. This occurs as the logical operand of if.Cc: Mateusz Kulikowski mateusz.kulikowski@gmail.com Reported-by: Coverity (CID: 143913)
[...]
- uint16_t pid; /* Peripheral ID on SPMI bus */
- uint32_t pid; /* Peripheral ID on SPMI bus */
};
static int pm8916_gpio_set_direction(struct udevice *dev, unsigned offset,
Note applies to two patches in this series (pm8916_gpio.c and pm8916.c)
I think (now, when the coverity pointed out mistake) that we should add in that case check if pid fits in 16-bits, as this is maximum pid value on spmi bus.
This checks should be done in pm8916_gpio_probe() and pm8916_probe().
Would you like to do it in your series or want me to post another patch on top of them?
Or even better - leave it as uint16_t and do check & cast in probe()
Further sanity checks on the data we get isn't a bad idea, so no casting.
On Mon, Apr 18, 2016 at 10:23:24PM +0200, Mateusz Kulikowski wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
Hi,
Sorry for slow reply.
On 12.04.2016 21:11, Tom Rini wrote:
If get_dev_addr fails it will return FDT_ADDR_T_NONE and:
"priv->pid == 4294967295U" is always false regardless of the values of its operands. This occurs as the logical operand of if.Cc: Mateusz Kulikowski mateusz.kulikowski@gmail.com Reported-by: Coverity (CID: 143913)
[...]
- uint16_t pid; /* Peripheral ID on SPMI bus */
- uint32_t pid; /* Peripheral ID on SPMI bus */
};
static int pm8916_gpio_set_direction(struct udevice *dev, unsigned offset,
Note applies to two patches in this series (pm8916_gpio.c and pm8916.c)
I think (now, when the coverity pointed out mistake) that we should add in that case check if pid fits in 16-bits, as this is maximum pid value on spmi bus.
This checks should be done in pm8916_gpio_probe() and pm8916_probe().
Would you like to do it in your series or want me to post another patch on top of them?
Please do a follow up, thanks!
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
Hi,
On 20.04.2016 23:25, Tom Rini wrote:
On Mon, Apr 18, 2016 at 10:23:24PM +0200, Mateusz Kulikowski wrote:
[..]
I think (now, when the coverity pointed out mistake) that we should add in that case check if pid fits in 16-bits, as this is maximum pid value on spmi bus.
This checks should be done in pm8916_gpio_probe() and pm8916_probe().
Would you like to do it in your series or want me to post another patch on top of them?
Please do a follow up, thanks!
Will do that, but it will take a few days as I have a lot of non-coding related assignments now :)
btw. SPMI core checks pid and sid internally during reads/writes so we're pretty safe anyway.
Regards, Mateusz
On Tue, Apr 12, 2016 at 03:11:23PM -0400, Tom Rini wrote:
If get_dev_addr fails it will return FDT_ADDR_T_NONE and:
"priv->pid == 4294967295U" is always false regardless of the values of its operands. This occurs as the logical operand of if.Cc: Mateusz Kulikowski mateusz.kulikowski@gmail.com Reported-by: Coverity (CID: 143913) Signed-off-by: Tom Rini trini@konsulko.com
Applied to u-boot/master, thanks!
If get_dev_addr fails it will return FDT_ADDR_T_NONE and:
"priv->usid == 4294967295U" is always false regardless of the values of its operands. This occurs as the logical operand of if.
Cc: Mateusz Kulikowski mateusz.kulikowski@gmail.com Reported-by: Coverity (CID: 143914) Signed-off-by: Tom Rini trini@konsulko.com --- drivers/power/pmic/pm8916.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/power/pmic/pm8916.c b/drivers/power/pmic/pm8916.c index 9acf5f5..d4c7d4a 100644 --- a/drivers/power/pmic/pm8916.c +++ b/drivers/power/pmic/pm8916.c @@ -18,7 +18,7 @@ DECLARE_GLOBAL_DATA_PTR; #define REG_MASK 0xFF
struct pm8916_priv { - uint16_t usid; /* Slave ID on SPMI bus */ + uint32_t usid; /* Slave ID on SPMI bus */ };
static int pm8916_reg_count(struct udevice *dev)
On Tue, Apr 12, 2016 at 03:11:24PM -0400, Tom Rini wrote:
If get_dev_addr fails it will return FDT_ADDR_T_NONE and:
"priv->usid == 4294967295U" is always false regardless of the values of its operands. This occurs as the logical operand of if.Cc: Mateusz Kulikowski mateusz.kulikowski@gmail.com Reported-by: Coverity (CID: 143914) Signed-off-by: Tom Rini trini@konsulko.com
Applied to u-boot/master, thanks!
On 12 April 2016 at 13:11, Tom Rini trini@konsulko.com wrote:
In rollback_space_kernel we were not initializing the reserved fields which should be for safety sake, and doing memset here means we don't need to set the version field specifically either.
Reported-by: Coverity (CID: 143917) Cc: Simon Glass sjg@chromium.org Signed-off-by: Tom Rini trini@konsulko.com
drivers/tpm/tpm_tis_sandbox.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
On 15 April 2016 at 08:12, Simon Glass sjg@chromium.org wrote:
On 12 April 2016 at 13:11, Tom Rini trini@konsulko.com wrote:
In rollback_space_kernel we were not initializing the reserved fields which should be for safety sake, and doing memset here means we don't need to set the version field specifically either.
Reported-by: Coverity (CID: 143917) Cc: Simon Glass sjg@chromium.org Signed-off-by: Tom Rini trini@konsulko.com
drivers/tpm/tpm_tis_sandbox.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot-dm, thanks!
participants (3)
-
Mateusz Kulikowski -
Simon Glass -
Tom Rini