[U-Boot] [PATCH 1/3] drivers/tpm/tpm_tis_sandbox.c: Fix uninitialized variable use

[U-Boot] [PATCH 1/3] drivers/tpm/tpm_tis_sandbox.c: Fix uninitialized variable use

[Tom Rini]

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));
-- 
1.9.1

[U-Boot] [PATCH 2/3] drivers/gpio/pm8916_gpio.c: Make pid be uint32_t

[Tom Rini]

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,
-- 
1.9.1

[U-Boot] [PATCH 2/3] drivers/gpio/pm8916_gpio.c: Make pid be uint32_t

[Mateusz Kulikowski]

-----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 SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJXFUIWAAoJELvtohmVtQzBow8H/i4ojMv7MM+4YdHZs/Tf1gdy
LWh68je+eGX+Nt1pwVXdgh52NJ6/PFJvUstlHaPKpPhlCAfOEEno6aI05dp0UQ5E
VG5bqyL1k1IoqDue+3xoL2tJ0IZvBTKDpeqw8rVDH5PEFp5/aXs9FKwZm9VcVpny
+3fi0sxFACAOs+PjMPqP9meXVTZyRRCTh/dAeH4bwLRGp249Tgq50vMajYjq1CW5
SM5TGFKeUgs17xVs163INW5s6UnNmqjj+gmVr7HRDH2uDWoQ6P77U9Z+l4yvSb72
Fw/vi6pqnxWsj6DCBEBkZ+jY2SVGB4LXm3bhQH2FO6ljXAW7KYamt1w4DoD9ABU=
=bczK
-----END PGP SIGNATURE-----

[U-Boot] [PATCH 2/3] drivers/gpio/pm8916_gpio.c: Make pid be uint32_t

[Mateusz Kulikowski]

-----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
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJXFUJtAAoJELvtohmVtQzBblgH/ROdYlG668Kz8ak/sUo/xksn
NFeWMDC8pORbCOLhPnHcEJhAPzx+a0c5pUWUvsloTnj8t08rM+pcmnVV+jW4DhAC
TxUgXYKjXMzbNlYYGn4cfb1Qlyss0+/xkv3C33LwI4hQYmE46RtThYtZqt6Uk+lk
9/eGCHt+H1tbKoD1BZIbzUwoDMTGlv7FgpzsH0/iW1QsDhbfUc27jkZNUvDgZO3Y
ElX9I9cSFD5oqSVO3X+iDtGXaD6UEOGU+nb42wkTc9gyg9qunS9TV2kJsrrJu2DD
Le4mN8NevrxyYbRZZM7EThkBD7skHr8wKUm6ccoQZLuMK+ZaYaukGirYTFSBQS0=
=MwzG
-----END PGP SIGNATURE-----

[U-Boot] [PATCH 2/3] drivers/gpio/pm8916_gpio.c: Make pid be uint32_t

[Tom Rini]

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.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160420/4578e853/attachment.sig>

[U-Boot] [PATCH 2/3] drivers/gpio/pm8916_gpio.c: Make pid be uint32_t

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160420/ba042fa9/attachment.sig>

[U-Boot] [PATCH 2/3] drivers/gpio/pm8916_gpio.c: Make pid be uint32_t

[Mateusz Kulikowski]

-----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
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJXGjE2AAoJELvtohmVtQzBMj4H/3+zypiWH9BZdDheztNA6yZt
vN2fIz9kWQB8GFoBktU7kRAAWXaTDB0CWsKQmcqnJtZJeqyUV0Dj1CEJoS4fcptD
Lq0uljuufLkfkSb/XhRe5sLxWbvygHVrGBz/OemFCQivBlu8MEe4pwNyEUFwTHrc
otF9Uk1y92HjsyaQhjMX6WWR5Ei8hrVSryppt1eRFoGBAw9uL7TcKnc96IKrbFZ+
ns/40n/K8O2k8J2JMxMXB1QbJj/lEj/y4yY2Il2B2Z3WHb4ZhErtoko55uh0bGf4
PoGH1wPTZxNrcKn0CCwnHxmIxGJMpJ8XyZYhsEI4J+ZBs7izzb4dSn8ekOsr1nI=
=Y2ca
-----END PGP SIGNATURE-----

[U-Boot] [U-Boot, 2/3] drivers/gpio/pm8916_gpio.c: Make pid be uint32_t

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160421/20ed5d78/attachment.sig>

[U-Boot] [PATCH 3/3] drivers/power/pmic/pm8916.c: Make usid be uint32_t

[Tom Rini]

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)
-- 
1.9.1

[U-Boot] [U-Boot, 3/3] drivers/power/pmic/pm8916.c: Make usid be uint32_t

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160421/a99c38e2/attachment.sig>

[U-Boot] [PATCH 1/3] drivers/tpm/tpm_tis_sandbox.c: Fix uninitialized variable use

[Simon Glass]

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>

[U-Boot] [PATCH 1/3] drivers/tpm/tpm_tis_sandbox.c: Fix uninitialized variable use

[Simon Glass]

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!