* [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-11 20:31 ` ndesaulniers
0 siblings, 0 replies; 68+ messages in thread
From: ndesaulniers @ 2018-10-11 20:31 UTC (permalink / raw)
To: jejb, dhowells
Cc: natechancellor, ebiggers, Nick Desaulniers, Mimi Zohar,
James Morris, Serge E. Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
by swapping h2 and h3.
security/keys/trusted.c:146:17: warning: passing an object that
undergoes default
argument promotion to 'va_start' has undefined behavior [-Wvarargs]
va_start(argp, h3);
^
security/keys/trusted.c:126:37: note: parameter of type 'unsigned
char' is declared here
unsigned char *h2, unsigned char h3, ...)
^
Specifically, it seems that both the C90 (4.8.1.1) and C11 (7.16.1.4)
standards explicitly call this out as undefined behavior:
The parameter parmN is the identifier of the rightmost parameter in
the variable parameter list in the function definition (the one just
before the ...). If the parameter parmN is declared with ... or with a
type that is not compatible with the type that results after
application of the default argument promotions, the behavior is
undefined.
Link: https://github.com/ClangBuiltLinux/linux/issues/41
Suggested-by: James Bottomley <jejb@linux.vnet.ibm.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
security/keys/trusted.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index b69d3b1777c2..d425b2b839af 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -123,7 +123,7 @@ static int TSS_rawhmac(unsigned char *digest, const unsigned char *key,
*/
static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
unsigned int keylen, unsigned char *h1,
- unsigned char *h2, unsigned char h3, ...)
+ unsigned char h2, unsigned char *h3, ...)
{
unsigned char paramdigest[SHA1_DIGEST_SIZE];
struct sdesc *sdesc;
@@ -139,7 +139,7 @@ static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
return PTR_ERR(sdesc);
}
- c = h3;
+ c = h2;
ret = crypto_shash_init(&sdesc->shash);
if (ret < 0)
goto out;
@@ -163,7 +163,7 @@ static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
if (!ret)
ret = TSS_rawhmac(digest, key, keylen, SHA1_DIGEST_SIZE,
paramdigest, TPM_NONCE_SIZE, h1,
- TPM_NONCE_SIZE, h2, 1, &c, 0, 0);
+ TPM_NONCE_SIZE, h3, 1, &c, 0, 0);
out:
kzfree(sdesc);
return ret;
@@ -508,7 +508,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
if (pcrinfosize == 0) {
/* no pcr info specified */
ret = TSS_authhmac(td->pubauth, sess.secret, SHA1_DIGEST_SIZE,
- sess.enonce, td->nonceodd, cont,
+ sess.enonce, cont, td->nonceodd,
sizeof(uint32_t), &ordinal, SHA1_DIGEST_SIZE,
td->encauth, sizeof(uint32_t), &pcrsize,
sizeof(uint32_t), &datsize, datalen, data, 0,
@@ -516,7 +516,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
} else {
/* pcr info specified */
ret = TSS_authhmac(td->pubauth, sess.secret, SHA1_DIGEST_SIZE,
- sess.enonce, td->nonceodd, cont,
+ sess.enonce, cont, td->nonceodd,
sizeof(uint32_t), &ordinal, SHA1_DIGEST_SIZE,
td->encauth, sizeof(uint32_t), &pcrsize,
pcrinfosize, pcrinfo, sizeof(uint32_t),
@@ -608,12 +608,12 @@ static int tpm_unseal(struct tpm_buf *tb,
return ret;
}
ret = TSS_authhmac(authdata1, keyauth, TPM_NONCE_SIZE,
- enonce1, nonceodd, cont, sizeof(uint32_t),
+ enonce1, cont, nonceodd, sizeof(uint32_t),
&ordinal, bloblen, blob, 0, 0);
if (ret < 0)
return ret;
ret = TSS_authhmac(authdata2, blobauth, TPM_NONCE_SIZE,
- enonce2, nonceodd, cont, sizeof(uint32_t),
+ enonce2, cont, nonceodd, sizeof(uint32_t),
&ordinal, bloblen, blob, 0, 0);
if (ret < 0)
return ret;
--
2.19.0.605.g01d371f741-goog
^ permalink raw reply related [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
2018-10-11 20:31 ` ndesaulniers
@ 2018-10-12 1:50 ` Nathan Chancellor
-1 siblings, 0 replies; 68+ messages in thread
From: Nathan Chancellor @ 2018-10-12 1:50 UTC (permalink / raw)
To: ndesaulniers
Cc: jejb, dhowells, ebiggers, Mimi Zohar, James Morris,
Serge E. Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
On Thu, Oct 11, 2018 at 01:31:26PM -0700, ndesaulniers@google.com wrote:
> by swapping h2 and h3.
>
> security/keys/trusted.c:146:17: warning: passing an object that
> undergoes default
> argument promotion to 'va_start' has undefined behavior [-Wvarargs]
> va_start(argp, h3);
> ^
> security/keys/trusted.c:126:37: note: parameter of type 'unsigned
> char' is declared here
> unsigned char *h2, unsigned char h3, ...)
> ^
>
> Specifically, it seems that both the C90 (4.8.1.1) and C11 (7.16.1.4)
> standards explicitly call this out as undefined behavior:
>
> The parameter parmN is the identifier of the rightmost parameter in
> the variable parameter list in the function definition (the one just
> before the ...). If the parameter parmN is declared with ... or with a
> type that is not compatible with the type that results after
> application of the default argument promotions, the behavior is
> undefined.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/41
> Suggested-by: James Bottomley <jejb@linux.vnet.ibm.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> security/keys/trusted.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/security/keys/trusted.c b/security/keys/trusted.c
> index b69d3b1777c2..d425b2b839af 100644
> --- a/security/keys/trusted.c
> +++ b/security/keys/trusted.c
> @@ -123,7 +123,7 @@ static int TSS_rawhmac(unsigned char *digest, const unsigned char *key,
> */
> static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
> unsigned int keylen, unsigned char *h1,
> - unsigned char *h2, unsigned char h3, ...)
> + unsigned char h2, unsigned char *h3, ...)
Prototype needs to be updated in include/keys/trusted.h and it looks
like this function is used in crypto/asymmetric_keys/asym_tpm.c so these
same changes should be made there.
Otherwise, looks good to me! Thanks for sending the patch.
Nathan
> {
> unsigned char paramdigest[SHA1_DIGEST_SIZE];
> struct sdesc *sdesc;
> @@ -139,7 +139,7 @@ static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
> return PTR_ERR(sdesc);
> }
>
> - c = h3;
> + c = h2;
> ret = crypto_shash_init(&sdesc->shash);
> if (ret < 0)
> goto out;
> @@ -163,7 +163,7 @@ static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
> if (!ret)
> ret = TSS_rawhmac(digest, key, keylen, SHA1_DIGEST_SIZE,
> paramdigest, TPM_NONCE_SIZE, h1,
> - TPM_NONCE_SIZE, h2, 1, &c, 0, 0);
> + TPM_NONCE_SIZE, h3, 1, &c, 0, 0);
> out:
> kzfree(sdesc);
> return ret;
> @@ -508,7 +508,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
> if (pcrinfosize = 0) {
> /* no pcr info specified */
> ret = TSS_authhmac(td->pubauth, sess.secret, SHA1_DIGEST_SIZE,
> - sess.enonce, td->nonceodd, cont,
> + sess.enonce, cont, td->nonceodd,
> sizeof(uint32_t), &ordinal, SHA1_DIGEST_SIZE,
> td->encauth, sizeof(uint32_t), &pcrsize,
> sizeof(uint32_t), &datsize, datalen, data, 0,
> @@ -516,7 +516,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
> } else {
> /* pcr info specified */
> ret = TSS_authhmac(td->pubauth, sess.secret, SHA1_DIGEST_SIZE,
> - sess.enonce, td->nonceodd, cont,
> + sess.enonce, cont, td->nonceodd,
> sizeof(uint32_t), &ordinal, SHA1_DIGEST_SIZE,
> td->encauth, sizeof(uint32_t), &pcrsize,
> pcrinfosize, pcrinfo, sizeof(uint32_t),
> @@ -608,12 +608,12 @@ static int tpm_unseal(struct tpm_buf *tb,
> return ret;
> }
> ret = TSS_authhmac(authdata1, keyauth, TPM_NONCE_SIZE,
> - enonce1, nonceodd, cont, sizeof(uint32_t),
> + enonce1, cont, nonceodd, sizeof(uint32_t),
> &ordinal, bloblen, blob, 0, 0);
> if (ret < 0)
> return ret;
> ret = TSS_authhmac(authdata2, blobauth, TPM_NONCE_SIZE,
> - enonce2, nonceodd, cont, sizeof(uint32_t),
> + enonce2, cont, nonceodd, sizeof(uint32_t),
> &ordinal, bloblen, blob, 0, 0);
> if (ret < 0)
> return ret;
> --
> 2.19.0.605.g01d371f741-goog
>
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-12 1:50 ` Nathan Chancellor
0 siblings, 0 replies; 68+ messages in thread
From: Nathan Chancellor @ 2018-10-12 1:50 UTC (permalink / raw)
To: ndesaulniers
Cc: jejb, dhowells, ebiggers, Mimi Zohar, James Morris,
Serge E. Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
On Thu, Oct 11, 2018 at 01:31:26PM -0700, ndesaulniers@google.com wrote:
> by swapping h2 and h3.
>
> security/keys/trusted.c:146:17: warning: passing an object that
> undergoes default
> argument promotion to 'va_start' has undefined behavior [-Wvarargs]
> va_start(argp, h3);
> ^
> security/keys/trusted.c:126:37: note: parameter of type 'unsigned
> char' is declared here
> unsigned char *h2, unsigned char h3, ...)
> ^
>
> Specifically, it seems that both the C90 (4.8.1.1) and C11 (7.16.1.4)
> standards explicitly call this out as undefined behavior:
>
> The parameter parmN is the identifier of the rightmost parameter in
> the variable parameter list in the function definition (the one just
> before the ...). If the parameter parmN is declared with ... or with a
> type that is not compatible with the type that results after
> application of the default argument promotions, the behavior is
> undefined.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/41
> Suggested-by: James Bottomley <jejb@linux.vnet.ibm.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> security/keys/trusted.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/security/keys/trusted.c b/security/keys/trusted.c
> index b69d3b1777c2..d425b2b839af 100644
> --- a/security/keys/trusted.c
> +++ b/security/keys/trusted.c
> @@ -123,7 +123,7 @@ static int TSS_rawhmac(unsigned char *digest, const unsigned char *key,
> */
> static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
> unsigned int keylen, unsigned char *h1,
> - unsigned char *h2, unsigned char h3, ...)
> + unsigned char h2, unsigned char *h3, ...)
Prototype needs to be updated in include/keys/trusted.h and it looks
like this function is used in crypto/asymmetric_keys/asym_tpm.c so these
same changes should be made there.
Otherwise, looks good to me! Thanks for sending the patch.
Nathan
> {
> unsigned char paramdigest[SHA1_DIGEST_SIZE];
> struct sdesc *sdesc;
> @@ -139,7 +139,7 @@ static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
> return PTR_ERR(sdesc);
> }
>
> - c = h3;
> + c = h2;
> ret = crypto_shash_init(&sdesc->shash);
> if (ret < 0)
> goto out;
> @@ -163,7 +163,7 @@ static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
> if (!ret)
> ret = TSS_rawhmac(digest, key, keylen, SHA1_DIGEST_SIZE,
> paramdigest, TPM_NONCE_SIZE, h1,
> - TPM_NONCE_SIZE, h2, 1, &c, 0, 0);
> + TPM_NONCE_SIZE, h3, 1, &c, 0, 0);
> out:
> kzfree(sdesc);
> return ret;
> @@ -508,7 +508,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
> if (pcrinfosize == 0) {
> /* no pcr info specified */
> ret = TSS_authhmac(td->pubauth, sess.secret, SHA1_DIGEST_SIZE,
> - sess.enonce, td->nonceodd, cont,
> + sess.enonce, cont, td->nonceodd,
> sizeof(uint32_t), &ordinal, SHA1_DIGEST_SIZE,
> td->encauth, sizeof(uint32_t), &pcrsize,
> sizeof(uint32_t), &datsize, datalen, data, 0,
> @@ -516,7 +516,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
> } else {
> /* pcr info specified */
> ret = TSS_authhmac(td->pubauth, sess.secret, SHA1_DIGEST_SIZE,
> - sess.enonce, td->nonceodd, cont,
> + sess.enonce, cont, td->nonceodd,
> sizeof(uint32_t), &ordinal, SHA1_DIGEST_SIZE,
> td->encauth, sizeof(uint32_t), &pcrsize,
> pcrinfosize, pcrinfo, sizeof(uint32_t),
> @@ -608,12 +608,12 @@ static int tpm_unseal(struct tpm_buf *tb,
> return ret;
> }
> ret = TSS_authhmac(authdata1, keyauth, TPM_NONCE_SIZE,
> - enonce1, nonceodd, cont, sizeof(uint32_t),
> + enonce1, cont, nonceodd, sizeof(uint32_t),
> &ordinal, bloblen, blob, 0, 0);
> if (ret < 0)
> return ret;
> ret = TSS_authhmac(authdata2, blobauth, TPM_NONCE_SIZE,
> - enonce2, nonceodd, cont, sizeof(uint32_t),
> + enonce2, cont, nonceodd, sizeof(uint32_t),
> &ordinal, bloblen, blob, 0, 0);
> if (ret < 0)
> return ret;
> --
> 2.19.0.605.g01d371f741-goog
>
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
2018-10-12 1:50 ` Nathan Chancellor
@ 2018-10-12 16:55 ` Nick Desaulniers
-1 siblings, 0 replies; 68+ messages in thread
From: Nick Desaulniers @ 2018-10-12 16:55 UTC (permalink / raw)
To: Nathan Chancellor
Cc: James E.J. Bottomley, dhowells, Eric Biggers, zohar, jmorris,
serge, linux-integrity, keyrings, linux-security-module, LKML
On Thu, Oct 11, 2018 at 6:50 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Thu, Oct 11, 2018 at 01:31:26PM -0700, ndesaulniers@google.com wrote:
> > by swapping h2 and h3.
> >
> > security/keys/trusted.c:146:17: warning: passing an object that
> > undergoes default
> > argument promotion to 'va_start' has undefined behavior [-Wvarargs]
> > va_start(argp, h3);
> > ^
> > security/keys/trusted.c:126:37: note: parameter of type 'unsigned
> > char' is declared here
> > unsigned char *h2, unsigned char h3, ...)
> > ^
> >
> > Specifically, it seems that both the C90 (4.8.1.1) and C11 (7.16.1.4)
> > standards explicitly call this out as undefined behavior:
> >
> > The parameter parmN is the identifier of the rightmost parameter in
> > the variable parameter list in the function definition (the one just
> > before the ...). If the parameter parmN is declared with ... or with a
> > type that is not compatible with the type that results after
> > application of the default argument promotions, the behavior is
> > undefined.
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/41
> > Suggested-by: James Bottomley <jejb@linux.vnet.ibm.com>
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > ---
> > security/keys/trusted.c | 14 +++++++-------
> > 1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/security/keys/trusted.c b/security/keys/trusted.c
> > index b69d3b1777c2..d425b2b839af 100644
> > --- a/security/keys/trusted.c
> > +++ b/security/keys/trusted.c
> > @@ -123,7 +123,7 @@ static int TSS_rawhmac(unsigned char *digest, const unsigned char *key,
> > */
> > static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
> > unsigned int keylen, unsigned char *h1,
> > - unsigned char *h2, unsigned char h3, ...)
> > + unsigned char h2, unsigned char *h3, ...)
>
> Prototype needs to be updated in include/keys/trusted.h and it looks
> like this function is used in crypto/asymmetric_keys/asym_tpm.c so these
> same changes should be made there.
Thanks for the review. Which tree are you looking at? These files
don't exist in torvalds/linux, but maybe -next or the crypto tree have
expanded the number of call sites of this function?
>
> Otherwise, looks good to me! Thanks for sending the patch.
>
> Nathan
>
> > {
> > unsigned char paramdigest[SHA1_DIGEST_SIZE];
> > struct sdesc *sdesc;
> > @@ -139,7 +139,7 @@ static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
> > return PTR_ERR(sdesc);
> > }
> >
> > - c = h3;
> > + c = h2;
> > ret = crypto_shash_init(&sdesc->shash);
> > if (ret < 0)
> > goto out;
> > @@ -163,7 +163,7 @@ static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
> > if (!ret)
> > ret = TSS_rawhmac(digest, key, keylen, SHA1_DIGEST_SIZE,
> > paramdigest, TPM_NONCE_SIZE, h1,
> > - TPM_NONCE_SIZE, h2, 1, &c, 0, 0);
> > + TPM_NONCE_SIZE, h3, 1, &c, 0, 0);
> > out:
> > kzfree(sdesc);
> > return ret;
> > @@ -508,7 +508,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
> > if (pcrinfosize = 0) {
> > /* no pcr info specified */
> > ret = TSS_authhmac(td->pubauth, sess.secret, SHA1_DIGEST_SIZE,
> > - sess.enonce, td->nonceodd, cont,
> > + sess.enonce, cont, td->nonceodd,
> > sizeof(uint32_t), &ordinal, SHA1_DIGEST_SIZE,
> > td->encauth, sizeof(uint32_t), &pcrsize,
> > sizeof(uint32_t), &datsize, datalen, data, 0,
> > @@ -516,7 +516,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
> > } else {
> > /* pcr info specified */
> > ret = TSS_authhmac(td->pubauth, sess.secret, SHA1_DIGEST_SIZE,
> > - sess.enonce, td->nonceodd, cont,
> > + sess.enonce, cont, td->nonceodd,
> > sizeof(uint32_t), &ordinal, SHA1_DIGEST_SIZE,
> > td->encauth, sizeof(uint32_t), &pcrsize,
> > pcrinfosize, pcrinfo, sizeof(uint32_t),
> > @@ -608,12 +608,12 @@ static int tpm_unseal(struct tpm_buf *tb,
> > return ret;
> > }
> > ret = TSS_authhmac(authdata1, keyauth, TPM_NONCE_SIZE,
> > - enonce1, nonceodd, cont, sizeof(uint32_t),
> > + enonce1, cont, nonceodd, sizeof(uint32_t),
> > &ordinal, bloblen, blob, 0, 0);
> > if (ret < 0)
> > return ret;
> > ret = TSS_authhmac(authdata2, blobauth, TPM_NONCE_SIZE,
> > - enonce2, nonceodd, cont, sizeof(uint32_t),
> > + enonce2, cont, nonceodd, sizeof(uint32_t),
> > &ordinal, bloblen, blob, 0, 0);
> > if (ret < 0)
> > return ret;
> > --
> > 2.19.0.605.g01d371f741-goog
> >
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-12 16:55 ` Nick Desaulniers
0 siblings, 0 replies; 68+ messages in thread
From: Nick Desaulniers @ 2018-10-12 16:55 UTC (permalink / raw)
To: Nathan Chancellor
Cc: James E.J. Bottomley, dhowells, Eric Biggers, zohar, jmorris,
serge, linux-integrity, keyrings, linux-security-module, LKML
On Thu, Oct 11, 2018 at 6:50 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Thu, Oct 11, 2018 at 01:31:26PM -0700, ndesaulniers@google.com wrote:
> > by swapping h2 and h3.
> >
> > security/keys/trusted.c:146:17: warning: passing an object that
> > undergoes default
> > argument promotion to 'va_start' has undefined behavior [-Wvarargs]
> > va_start(argp, h3);
> > ^
> > security/keys/trusted.c:126:37: note: parameter of type 'unsigned
> > char' is declared here
> > unsigned char *h2, unsigned char h3, ...)
> > ^
> >
> > Specifically, it seems that both the C90 (4.8.1.1) and C11 (7.16.1.4)
> > standards explicitly call this out as undefined behavior:
> >
> > The parameter parmN is the identifier of the rightmost parameter in
> > the variable parameter list in the function definition (the one just
> > before the ...). If the parameter parmN is declared with ... or with a
> > type that is not compatible with the type that results after
> > application of the default argument promotions, the behavior is
> > undefined.
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/41
> > Suggested-by: James Bottomley <jejb@linux.vnet.ibm.com>
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > ---
> > security/keys/trusted.c | 14 +++++++-------
> > 1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/security/keys/trusted.c b/security/keys/trusted.c
> > index b69d3b1777c2..d425b2b839af 100644
> > --- a/security/keys/trusted.c
> > +++ b/security/keys/trusted.c
> > @@ -123,7 +123,7 @@ static int TSS_rawhmac(unsigned char *digest, const unsigned char *key,
> > */
> > static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
> > unsigned int keylen, unsigned char *h1,
> > - unsigned char *h2, unsigned char h3, ...)
> > + unsigned char h2, unsigned char *h3, ...)
>
> Prototype needs to be updated in include/keys/trusted.h and it looks
> like this function is used in crypto/asymmetric_keys/asym_tpm.c so these
> same changes should be made there.
Thanks for the review. Which tree are you looking at? These files
don't exist in torvalds/linux, but maybe -next or the crypto tree have
expanded the number of call sites of this function?
>
> Otherwise, looks good to me! Thanks for sending the patch.
>
> Nathan
>
> > {
> > unsigned char paramdigest[SHA1_DIGEST_SIZE];
> > struct sdesc *sdesc;
> > @@ -139,7 +139,7 @@ static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
> > return PTR_ERR(sdesc);
> > }
> >
> > - c = h3;
> > + c = h2;
> > ret = crypto_shash_init(&sdesc->shash);
> > if (ret < 0)
> > goto out;
> > @@ -163,7 +163,7 @@ static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
> > if (!ret)
> > ret = TSS_rawhmac(digest, key, keylen, SHA1_DIGEST_SIZE,
> > paramdigest, TPM_NONCE_SIZE, h1,
> > - TPM_NONCE_SIZE, h2, 1, &c, 0, 0);
> > + TPM_NONCE_SIZE, h3, 1, &c, 0, 0);
> > out:
> > kzfree(sdesc);
> > return ret;
> > @@ -508,7 +508,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
> > if (pcrinfosize == 0) {
> > /* no pcr info specified */
> > ret = TSS_authhmac(td->pubauth, sess.secret, SHA1_DIGEST_SIZE,
> > - sess.enonce, td->nonceodd, cont,
> > + sess.enonce, cont, td->nonceodd,
> > sizeof(uint32_t), &ordinal, SHA1_DIGEST_SIZE,
> > td->encauth, sizeof(uint32_t), &pcrsize,
> > sizeof(uint32_t), &datsize, datalen, data, 0,
> > @@ -516,7 +516,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
> > } else {
> > /* pcr info specified */
> > ret = TSS_authhmac(td->pubauth, sess.secret, SHA1_DIGEST_SIZE,
> > - sess.enonce, td->nonceodd, cont,
> > + sess.enonce, cont, td->nonceodd,
> > sizeof(uint32_t), &ordinal, SHA1_DIGEST_SIZE,
> > td->encauth, sizeof(uint32_t), &pcrsize,
> > pcrinfosize, pcrinfo, sizeof(uint32_t),
> > @@ -608,12 +608,12 @@ static int tpm_unseal(struct tpm_buf *tb,
> > return ret;
> > }
> > ret = TSS_authhmac(authdata1, keyauth, TPM_NONCE_SIZE,
> > - enonce1, nonceodd, cont, sizeof(uint32_t),
> > + enonce1, cont, nonceodd, sizeof(uint32_t),
> > &ordinal, bloblen, blob, 0, 0);
> > if (ret < 0)
> > return ret;
> > ret = TSS_authhmac(authdata2, blobauth, TPM_NONCE_SIZE,
> > - enonce2, nonceodd, cont, sizeof(uint32_t),
> > + enonce2, cont, nonceodd, sizeof(uint32_t),
> > &ordinal, bloblen, blob, 0, 0);
> > if (ret < 0)
> > return ret;
> > --
> > 2.19.0.605.g01d371f741-goog
> >
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
2018-10-12 16:55 ` Nick Desaulniers
@ 2018-10-12 17:03 ` Nathan Chancellor
-1 siblings, 0 replies; 68+ messages in thread
From: Nathan Chancellor @ 2018-10-12 17:03 UTC (permalink / raw)
To: Nick Desaulniers
Cc: James E.J. Bottomley, dhowells, Eric Biggers, zohar, jmorris,
serge, linux-integrity, keyrings, linux-security-module, LKML
On Fri, Oct 12, 2018 at 09:55:55AM -0700, Nick Desaulniers wrote:
> On Thu, Oct 11, 2018 at 6:50 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > On Thu, Oct 11, 2018 at 01:31:26PM -0700, ndesaulniers@google.com wrote:
> > > by swapping h2 and h3.
> > >
> > > security/keys/trusted.c:146:17: warning: passing an object that
> > > undergoes default
> > > argument promotion to 'va_start' has undefined behavior [-Wvarargs]
> > > va_start(argp, h3);
> > > ^
> > > security/keys/trusted.c:126:37: note: parameter of type 'unsigned
> > > char' is declared here
> > > unsigned char *h2, unsigned char h3, ...)
> > > ^
> > >
> > > Specifically, it seems that both the C90 (4.8.1.1) and C11 (7.16.1.4)
> > > standards explicitly call this out as undefined behavior:
> > >
> > > The parameter parmN is the identifier of the rightmost parameter in
> > > the variable parameter list in the function definition (the one just
> > > before the ...). If the parameter parmN is declared with ... or with a
> > > type that is not compatible with the type that results after
> > > application of the default argument promotions, the behavior is
> > > undefined.
> > >
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/41
> > > Suggested-by: James Bottomley <jejb@linux.vnet.ibm.com>
> > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > > ---
> > > security/keys/trusted.c | 14 +++++++-------
> > > 1 file changed, 7 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/security/keys/trusted.c b/security/keys/trusted.c
> > > index b69d3b1777c2..d425b2b839af 100644
> > > --- a/security/keys/trusted.c
> > > +++ b/security/keys/trusted.c
> > > @@ -123,7 +123,7 @@ static int TSS_rawhmac(unsigned char *digest, const unsigned char *key,
> > > */
> > > static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
> > > unsigned int keylen, unsigned char *h1,
> > > - unsigned char *h2, unsigned char h3, ...)
> > > + unsigned char h2, unsigned char *h3, ...)
> >
> > Prototype needs to be updated in include/keys/trusted.h and it looks
> > like this function is used in crypto/asymmetric_keys/asym_tpm.c so these
> > same changes should be made there.
>
> Thanks for the review. Which tree are you looking at? These files
> don't exist in torvalds/linux, but maybe -next or the crypto tree have
> expanded the number of call sites of this function?
>
Yes, sorry I always work off of -next. Looks like commit 67714f79c8f7 ("KEYS:
trusted: Expose common functionality [ver #2]") exposes the function in
trusted.h and commit 27728d92a7df ("KEYS: asym_tpm: Add loadkey2 and
flushspecific [ver #2]") uses it. They're currently in -next, from the next-keys
branch in the security tree.
> >
> > Otherwise, looks good to me! Thanks for sending the patch.
> >
> > Nathan
> >
> > > {
> > > unsigned char paramdigest[SHA1_DIGEST_SIZE];
> > > struct sdesc *sdesc;
> > > @@ -139,7 +139,7 @@ static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
> > > return PTR_ERR(sdesc);
> > > }
> > >
> > > - c = h3;
> > > + c = h2;
> > > ret = crypto_shash_init(&sdesc->shash);
> > > if (ret < 0)
> > > goto out;
> > > @@ -163,7 +163,7 @@ static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
> > > if (!ret)
> > > ret = TSS_rawhmac(digest, key, keylen, SHA1_DIGEST_SIZE,
> > > paramdigest, TPM_NONCE_SIZE, h1,
> > > - TPM_NONCE_SIZE, h2, 1, &c, 0, 0);
> > > + TPM_NONCE_SIZE, h3, 1, &c, 0, 0);
> > > out:
> > > kzfree(sdesc);
> > > return ret;
> > > @@ -508,7 +508,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
> > > if (pcrinfosize = 0) {
> > > /* no pcr info specified */
> > > ret = TSS_authhmac(td->pubauth, sess.secret, SHA1_DIGEST_SIZE,
> > > - sess.enonce, td->nonceodd, cont,
> > > + sess.enonce, cont, td->nonceodd,
> > > sizeof(uint32_t), &ordinal, SHA1_DIGEST_SIZE,
> > > td->encauth, sizeof(uint32_t), &pcrsize,
> > > sizeof(uint32_t), &datsize, datalen, data, 0,
> > > @@ -516,7 +516,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
> > > } else {
> > > /* pcr info specified */
> > > ret = TSS_authhmac(td->pubauth, sess.secret, SHA1_DIGEST_SIZE,
> > > - sess.enonce, td->nonceodd, cont,
> > > + sess.enonce, cont, td->nonceodd,
> > > sizeof(uint32_t), &ordinal, SHA1_DIGEST_SIZE,
> > > td->encauth, sizeof(uint32_t), &pcrsize,
> > > pcrinfosize, pcrinfo, sizeof(uint32_t),
> > > @@ -608,12 +608,12 @@ static int tpm_unseal(struct tpm_buf *tb,
> > > return ret;
> > > }
> > > ret = TSS_authhmac(authdata1, keyauth, TPM_NONCE_SIZE,
> > > - enonce1, nonceodd, cont, sizeof(uint32_t),
> > > + enonce1, cont, nonceodd, sizeof(uint32_t),
> > > &ordinal, bloblen, blob, 0, 0);
> > > if (ret < 0)
> > > return ret;
> > > ret = TSS_authhmac(authdata2, blobauth, TPM_NONCE_SIZE,
> > > - enonce2, nonceodd, cont, sizeof(uint32_t),
> > > + enonce2, cont, nonceodd, sizeof(uint32_t),
> > > &ordinal, bloblen, blob, 0, 0);
> > > if (ret < 0)
> > > return ret;
> > > --
> > > 2.19.0.605.g01d371f741-goog
> > >
>
>
>
> --
> Thanks,
> ~Nick Desaulniers
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-12 17:03 ` Nathan Chancellor
0 siblings, 0 replies; 68+ messages in thread
From: Nathan Chancellor @ 2018-10-12 17:03 UTC (permalink / raw)
To: Nick Desaulniers
Cc: James E.J. Bottomley, dhowells, Eric Biggers, zohar, jmorris,
serge, linux-integrity, keyrings, linux-security-module, LKML
On Fri, Oct 12, 2018 at 09:55:55AM -0700, Nick Desaulniers wrote:
> On Thu, Oct 11, 2018 at 6:50 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > On Thu, Oct 11, 2018 at 01:31:26PM -0700, ndesaulniers@google.com wrote:
> > > by swapping h2 and h3.
> > >
> > > security/keys/trusted.c:146:17: warning: passing an object that
> > > undergoes default
> > > argument promotion to 'va_start' has undefined behavior [-Wvarargs]
> > > va_start(argp, h3);
> > > ^
> > > security/keys/trusted.c:126:37: note: parameter of type 'unsigned
> > > char' is declared here
> > > unsigned char *h2, unsigned char h3, ...)
> > > ^
> > >
> > > Specifically, it seems that both the C90 (4.8.1.1) and C11 (7.16.1.4)
> > > standards explicitly call this out as undefined behavior:
> > >
> > > The parameter parmN is the identifier of the rightmost parameter in
> > > the variable parameter list in the function definition (the one just
> > > before the ...). If the parameter parmN is declared with ... or with a
> > > type that is not compatible with the type that results after
> > > application of the default argument promotions, the behavior is
> > > undefined.
> > >
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/41
> > > Suggested-by: James Bottomley <jejb@linux.vnet.ibm.com>
> > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > > ---
> > > security/keys/trusted.c | 14 +++++++-------
> > > 1 file changed, 7 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/security/keys/trusted.c b/security/keys/trusted.c
> > > index b69d3b1777c2..d425b2b839af 100644
> > > --- a/security/keys/trusted.c
> > > +++ b/security/keys/trusted.c
> > > @@ -123,7 +123,7 @@ static int TSS_rawhmac(unsigned char *digest, const unsigned char *key,
> > > */
> > > static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
> > > unsigned int keylen, unsigned char *h1,
> > > - unsigned char *h2, unsigned char h3, ...)
> > > + unsigned char h2, unsigned char *h3, ...)
> >
> > Prototype needs to be updated in include/keys/trusted.h and it looks
> > like this function is used in crypto/asymmetric_keys/asym_tpm.c so these
> > same changes should be made there.
>
> Thanks for the review. Which tree are you looking at? These files
> don't exist in torvalds/linux, but maybe -next or the crypto tree have
> expanded the number of call sites of this function?
>
Yes, sorry I always work off of -next. Looks like commit 67714f79c8f7 ("KEYS:
trusted: Expose common functionality [ver #2]") exposes the function in
trusted.h and commit 27728d92a7df ("KEYS: asym_tpm: Add loadkey2 and
flushspecific [ver #2]") uses it. They're currently in -next, from the next-keys
branch in the security tree.
> >
> > Otherwise, looks good to me! Thanks for sending the patch.
> >
> > Nathan
> >
> > > {
> > > unsigned char paramdigest[SHA1_DIGEST_SIZE];
> > > struct sdesc *sdesc;
> > > @@ -139,7 +139,7 @@ static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
> > > return PTR_ERR(sdesc);
> > > }
> > >
> > > - c = h3;
> > > + c = h2;
> > > ret = crypto_shash_init(&sdesc->shash);
> > > if (ret < 0)
> > > goto out;
> > > @@ -163,7 +163,7 @@ static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
> > > if (!ret)
> > > ret = TSS_rawhmac(digest, key, keylen, SHA1_DIGEST_SIZE,
> > > paramdigest, TPM_NONCE_SIZE, h1,
> > > - TPM_NONCE_SIZE, h2, 1, &c, 0, 0);
> > > + TPM_NONCE_SIZE, h3, 1, &c, 0, 0);
> > > out:
> > > kzfree(sdesc);
> > > return ret;
> > > @@ -508,7 +508,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
> > > if (pcrinfosize == 0) {
> > > /* no pcr info specified */
> > > ret = TSS_authhmac(td->pubauth, sess.secret, SHA1_DIGEST_SIZE,
> > > - sess.enonce, td->nonceodd, cont,
> > > + sess.enonce, cont, td->nonceodd,
> > > sizeof(uint32_t), &ordinal, SHA1_DIGEST_SIZE,
> > > td->encauth, sizeof(uint32_t), &pcrsize,
> > > sizeof(uint32_t), &datsize, datalen, data, 0,
> > > @@ -516,7 +516,7 @@ static int tpm_seal(struct tpm_buf *tb, uint16_t keytype,
> > > } else {
> > > /* pcr info specified */
> > > ret = TSS_authhmac(td->pubauth, sess.secret, SHA1_DIGEST_SIZE,
> > > - sess.enonce, td->nonceodd, cont,
> > > + sess.enonce, cont, td->nonceodd,
> > > sizeof(uint32_t), &ordinal, SHA1_DIGEST_SIZE,
> > > td->encauth, sizeof(uint32_t), &pcrsize,
> > > pcrinfosize, pcrinfo, sizeof(uint32_t),
> > > @@ -608,12 +608,12 @@ static int tpm_unseal(struct tpm_buf *tb,
> > > return ret;
> > > }
> > > ret = TSS_authhmac(authdata1, keyauth, TPM_NONCE_SIZE,
> > > - enonce1, nonceodd, cont, sizeof(uint32_t),
> > > + enonce1, cont, nonceodd, sizeof(uint32_t),
> > > &ordinal, bloblen, blob, 0, 0);
> > > if (ret < 0)
> > > return ret;
> > > ret = TSS_authhmac(authdata2, blobauth, TPM_NONCE_SIZE,
> > > - enonce2, nonceodd, cont, sizeof(uint32_t),
> > > + enonce2, cont, nonceodd, sizeof(uint32_t),
> > > &ordinal, bloblen, blob, 0, 0);
> > > if (ret < 0)
> > > return ret;
> > > --
> > > 2.19.0.605.g01d371f741-goog
> > >
>
>
>
> --
> Thanks,
> ~Nick Desaulniers
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
2018-10-11 20:31 ` ndesaulniers
@ 2018-10-12 12:29 ` Denis Kenzior
-1 siblings, 0 replies; 68+ messages in thread
From: Denis Kenzior @ 2018-10-12 12:29 UTC (permalink / raw)
To: ndesaulniers, jejb, dhowells
Cc: natechancellor, ebiggers, Mimi Zohar, James Morris,
Serge E. Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
Hi Nick,
> @@ -123,7 +123,7 @@ static int TSS_rawhmac(unsigned char *digest, const unsigned char *key,
> */
> static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
> unsigned int keylen, unsigned char *h1,
> - unsigned char *h2, unsigned char h3, ...)
> + unsigned char h2, unsigned char *h3, ...)
> {
> unsigned char paramdigest[SHA1_DIGEST_SIZE];
> struct sdesc *sdesc;
So my concern here is that this actually breaks the natural argument
order compared to what the specification uses. This in turn requires
one to perform some mental gymnastics and I'm not sure that this is such
a good idea. Refer to
https://trustedcomputinggroup.org/wp-content/uploads/TPM-Main-Part-3-Commands_v1.2_rev116_01032011.pdf
for details.
Note that H3 is really the 'continueAuthSession' variable which is a
bool. In the above specification BOOL has a size of 1, and TSS_authhmac
already assigns a h3 to 'c' which is used for the actual hashing.
So can't we simply use 'bool' or uint32 as the type for h3 instead of
re-ordering everything?
Regards,
-Denis
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-12 12:29 ` Denis Kenzior
0 siblings, 0 replies; 68+ messages in thread
From: Denis Kenzior @ 2018-10-12 12:29 UTC (permalink / raw)
To: ndesaulniers, jejb, dhowells
Cc: natechancellor, ebiggers, Mimi Zohar, James Morris,
Serge E. Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
Hi Nick,
> @@ -123,7 +123,7 @@ static int TSS_rawhmac(unsigned char *digest, const unsigned char *key,
> */
> static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
> unsigned int keylen, unsigned char *h1,
> - unsigned char *h2, unsigned char h3, ...)
> + unsigned char h2, unsigned char *h3, ...)
> {
> unsigned char paramdigest[SHA1_DIGEST_SIZE];
> struct sdesc *sdesc;
So my concern here is that this actually breaks the natural argument
order compared to what the specification uses. This in turn requires
one to perform some mental gymnastics and I'm not sure that this is such
a good idea. Refer to
https://trustedcomputinggroup.org/wp-content/uploads/TPM-Main-Part-3-Commands_v1.2_rev116_01032011.pdf
for details.
Note that H3 is really the 'continueAuthSession' variable which is a
bool. In the above specification BOOL has a size of 1, and TSS_authhmac
already assigns a h3 to 'c' which is used for the actual hashing.
So can't we simply use 'bool' or uint32 as the type for h3 instead of
re-ordering everything?
Regards,
-Denis
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
2018-10-12 12:29 ` Denis Kenzior
@ 2018-10-12 15:05 ` James Bottomley
-1 siblings, 0 replies; 68+ messages in thread
From: James Bottomley @ 2018-10-12 15:05 UTC (permalink / raw)
To: Denis Kenzior, ndesaulniers, dhowells
Cc: natechancellor, ebiggers, Mimi Zohar, James Morris,
Serge E. Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
On Fri, 2018-10-12 at 07:29 -0500, Denis Kenzior wrote:
> Hi Nick,
>
> > @@ -123,7 +123,7 @@ static int TSS_rawhmac(unsigned char *digest,
> > const unsigned char *key,
> > */
> > static int TSS_authhmac(unsigned char *digest, const unsigned
> > char *key,
> > unsigned int keylen, unsigned char *h1,
> > - unsigned char *h2, unsigned char h3, ...)
> > + unsigned char h2, unsigned char *h3, ...)
> > {
> > unsigned char paramdigest[SHA1_DIGEST_SIZE];
> > struct sdesc *sdesc;
>
> So my concern here is that this actually breaks the natural argument
> order compared to what the specification uses. This in turn
> requires
> one to perform some mental gymnastics and I'm not sure that this is
> such
> a good idea. Refer to
> https://trustedcomputinggroup.org/wp-content/uploads/TPM-Main-Part-3-
> Commands_v1.2_rev116_01032011.pdf
> for details.
>
> Note that H3 is really the 'continueAuthSession' variable which is a
> bool. In the above specification BOOL has a size of 1, and
> TSS_authhmac already assigns a h3 to 'c' which is used for the actual
> hashing.
>
> So can't we simply use 'bool' or uint32 as the type for h3 instead
> of re-ordering everything
The problem is the standard is ambiguious. The only thing that's
guaranteed to work for all time is a char *. If you want to keep the
order, what I'd suggest is inserting a dummy pointer argument which is
always expected to be NULL between the h3 and the varargs.
James
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-12 15:05 ` James Bottomley
0 siblings, 0 replies; 68+ messages in thread
From: James Bottomley @ 2018-10-12 15:05 UTC (permalink / raw)
To: Denis Kenzior, ndesaulniers, dhowells
Cc: natechancellor, ebiggers, Mimi Zohar, James Morris,
Serge E. Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
On Fri, 2018-10-12 at 07:29 -0500, Denis Kenzior wrote:
> Hi Nick,
>
> > @@ -123,7 +123,7 @@ static int TSS_rawhmac(unsigned char *digest,
> > const unsigned char *key,
> > */
> > static int TSS_authhmac(unsigned char *digest, const unsigned
> > char *key,
> > unsigned int keylen, unsigned char *h1,
> > - unsigned char *h2, unsigned char h3, ...)
> > + unsigned char h2, unsigned char *h3, ...)
> > {
> > unsigned char paramdigest[SHA1_DIGEST_SIZE];
> > struct sdesc *sdesc;
>
> So my concern here is that this actually breaks the natural argument
> order compared to what the specification uses. This in turn
> requires
> one to perform some mental gymnastics and I'm not sure that this is
> such
> a good idea. Refer to
> https://trustedcomputinggroup.org/wp-content/uploads/TPM-Main-Part-3-
> Commands_v1.2_rev116_01032011.pdf
> for details.
>
> Note that H3 is really the 'continueAuthSession' variable which is a
> bool. In the above specification BOOL has a size of 1, and
> TSS_authhmac already assigns a h3 to 'c' which is used for the actual
> hashing.
>
> So can't we simply use 'bool' or uint32 as the type for h3 instead
> of re-ordering everything
The problem is the standard is ambiguious. The only thing that's
guaranteed to work for all time is a char *. If you want to keep the
order, what I'd suggest is inserting a dummy pointer argument which is
always expected to be NULL between the h3 and the varargs.
James
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
2018-10-12 15:05 ` James Bottomley
@ 2018-10-12 15:13 ` Denis Kenzior
-1 siblings, 0 replies; 68+ messages in thread
From: Denis Kenzior @ 2018-10-12 15:13 UTC (permalink / raw)
To: James Bottomley, ndesaulniers, dhowells
Cc: natechancellor, ebiggers, Mimi Zohar, James Morris,
Serge E. Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
Hi James,
>> So can't we simply use 'bool' or uint32 as the type for h3 instead
>> of re-ordering everything
>
> The problem is the standard is ambiguious. The only thing that's
> guaranteed to work for all time is a char *. If you want to keep the
> order, what I'd suggest is inserting a dummy pointer argument which is
> always expected to be NULL between the h3 and the varargs.
So maybe I'm misunderstanding something, but the issue seems to be that
unsigned char is promoted to 'unsigned char *' by Clang and probably
unsigned int or int by gcc.
So instead of having unsigned char h3, can't we simply have bool h3 or
unsigned int h3?
Regards,
-Denis
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-12 15:13 ` Denis Kenzior
0 siblings, 0 replies; 68+ messages in thread
From: Denis Kenzior @ 2018-10-12 15:13 UTC (permalink / raw)
To: James Bottomley, ndesaulniers, dhowells
Cc: natechancellor, ebiggers, Mimi Zohar, James Morris,
Serge E. Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
Hi James,
>> So can't we simply use 'bool' or uint32 as the type for h3 instead
>> of re-ordering everything
>
> The problem is the standard is ambiguious. The only thing that's
> guaranteed to work for all time is a char *. If you want to keep the
> order, what I'd suggest is inserting a dummy pointer argument which is
> always expected to be NULL between the h3 and the varargs.
So maybe I'm misunderstanding something, but the issue seems to be that
unsigned char is promoted to 'unsigned char *' by Clang and probably
unsigned int or int by gcc.
So instead of having unsigned char h3, can't we simply have bool h3 or
unsigned int h3?
Regards,
-Denis
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
2018-10-12 15:13 ` Denis Kenzior
@ 2018-10-12 15:22 ` James Bottomley
-1 siblings, 0 replies; 68+ messages in thread
From: James Bottomley @ 2018-10-12 15:22 UTC (permalink / raw)
To: Denis Kenzior, ndesaulniers, dhowells
Cc: natechancellor, ebiggers, Mimi Zohar, James Morris,
Serge E. Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
On Fri, 2018-10-12 at 10:13 -0500, Denis Kenzior wrote:
> Hi James,
>
> > > So can't we simply use 'bool' or uint32 as the type for h3
> > > instead of re-ordering everything
> >
> > The problem is the standard is ambiguious. The only thing that's
> > guaranteed to work for all time is a char *. If you want to keep
> > the order, what I'd suggest is inserting a dummy pointer argument
> > which is always expected to be NULL between the h3 and the varargs.
>
> So maybe I'm misunderstanding something, but the issue seems to be
> that unsigned char is promoted to 'unsigned char *' by Clang and
> probably unsigned int or int by gcc.
>
> So instead of having unsigned char h3, can't we simply have bool h3
> or unsigned int h3?
Given the ambiguity in the standards, the safe thing that will work for
all time and all potential compilers is a char *
James
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-12 15:22 ` James Bottomley
0 siblings, 0 replies; 68+ messages in thread
From: James Bottomley @ 2018-10-12 15:22 UTC (permalink / raw)
To: Denis Kenzior, ndesaulniers, dhowells
Cc: natechancellor, ebiggers, Mimi Zohar, James Morris,
Serge E. Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
On Fri, 2018-10-12 at 10:13 -0500, Denis Kenzior wrote:
> Hi James,
>
> > > So can't we simply use 'bool' or uint32 as the type for h3
> > > instead of re-ordering everything
> >
> > The problem is the standard is ambiguious. The only thing that's
> > guaranteed to work for all time is a char *. If you want to keep
> > the order, what I'd suggest is inserting a dummy pointer argument
> > which is always expected to be NULL between the h3 and the varargs.
>
> So maybe I'm misunderstanding something, but the issue seems to be
> that unsigned char is promoted to 'unsigned char *' by Clang and
> probably unsigned int or int by gcc.
>
> So instead of having unsigned char h3, can't we simply have bool h3
> or unsigned int h3?
Given the ambiguity in the standards, the safe thing that will work for
all time and all potential compilers is a char *
James
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
2018-10-12 15:22 ` James Bottomley
@ 2018-10-12 15:44 ` Denis Kenzior
-1 siblings, 0 replies; 68+ messages in thread
From: Denis Kenzior @ 2018-10-12 15:44 UTC (permalink / raw)
To: James Bottomley, ndesaulniers, dhowells
Cc: natechancellor, ebiggers, Mimi Zohar, James Morris,
Serge E. Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
Hi James,
>> So instead of having unsigned char h3, can't we simply have bool h3
>> or unsigned int h3?
>
> Given the ambiguity in the standards, the safe thing that will work for
> all time and all potential compilers is a char *
>
All right. You state this with certainty, but I'd still like you to
educate me why?
From the links provided in the patch it seems that one cannot pass
char/float/short to va_start(). Fair enough. So if we make h3 an
unsigned int, the issue goes away, no?
int TSS_authhmac(unsigned char *digest, const unsigned char *key,
unsigned int keylen, unsigned char *h1,
- unsigned char *h2, unsigned char h3, ...);
+ unsigned char *h2, unsigned int h3, ...);
Regards,
-Denis
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-12 15:44 ` Denis Kenzior
0 siblings, 0 replies; 68+ messages in thread
From: Denis Kenzior @ 2018-10-12 15:44 UTC (permalink / raw)
To: James Bottomley, ndesaulniers, dhowells
Cc: natechancellor, ebiggers, Mimi Zohar, James Morris,
Serge E. Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
Hi James,
>> So instead of having unsigned char h3, can't we simply have bool h3
>> or unsigned int h3?
>
> Given the ambiguity in the standards, the safe thing that will work for
> all time and all potential compilers is a char *
>
All right. You state this with certainty, but I'd still like you to
educate me why?
From the links provided in the patch it seems that one cannot pass
char/float/short to va_start(). Fair enough. So if we make h3 an
unsigned int, the issue goes away, no?
int TSS_authhmac(unsigned char *digest, const unsigned char *key,
unsigned int keylen, unsigned char *h1,
- unsigned char *h2, unsigned char h3, ...);
+ unsigned char *h2, unsigned int h3, ...);
Regards,
-Denis
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
2018-10-12 15:44 ` Denis Kenzior
@ 2018-10-12 15:46 ` James Bottomley
-1 siblings, 0 replies; 68+ messages in thread
From: James Bottomley @ 2018-10-12 15:46 UTC (permalink / raw)
To: Denis Kenzior, ndesaulniers, dhowells
Cc: natechancellor, ebiggers, Mimi Zohar, James Morris,
Serge E. Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
On Fri, 2018-10-12 at 10:44 -0500, Denis Kenzior wrote:
> Hi James,
>
> > > So instead of having unsigned char h3, can't we simply have bool
> > > h3 or unsigned int h3?
> >
> > Given the ambiguity in the standards, the safe thing that will work
> > for all time and all potential compilers is a char *
> >
>
> All right. You state this with certainty, but I'd still like you to
> educate me why?
>
> From the links provided in the patch it seems that one cannot pass
> char/float/short to va_start(). Fair enough. So if we make h3 an
> unsigned int, the issue goes away, no?
For the current version of clang, yes. However, if we're fixing this
for good a char * pointer is the only guaranteed thing because it
mirrors current use in printf.
James
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-12 15:46 ` James Bottomley
0 siblings, 0 replies; 68+ messages in thread
From: James Bottomley @ 2018-10-12 15:46 UTC (permalink / raw)
To: Denis Kenzior, ndesaulniers, dhowells
Cc: natechancellor, ebiggers, Mimi Zohar, James Morris,
Serge E. Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
On Fri, 2018-10-12 at 10:44 -0500, Denis Kenzior wrote:
> Hi James,
>
> > > So instead of having unsigned char h3, can't we simply have bool
> > > h3 or unsigned int h3?
> >
> > Given the ambiguity in the standards, the safe thing that will work
> > for all time and all potential compilers is a char *
> >
>
> All right. You state this with certainty, but I'd still like you to
> educate me why?
>
> From the links provided in the patch it seems that one cannot pass
> char/float/short to va_start(). Fair enough. So if we make h3 an
> unsigned int, the issue goes away, no?
For the current version of clang, yes. However, if we're fixing this
for good a char * pointer is the only guaranteed thing because it
mirrors current use in printf.
James
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
2018-10-12 15:46 ` James Bottomley
@ 2018-10-12 15:53 ` Denis Kenzior
-1 siblings, 0 replies; 68+ messages in thread
From: Denis Kenzior @ 2018-10-12 15:53 UTC (permalink / raw)
To: James Bottomley, ndesaulniers, dhowells
Cc: natechancellor, ebiggers, Mimi Zohar, James Morris,
Serge E. Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
Hi James,
>> From the links provided in the patch it seems that one cannot pass
>> char/float/short to va_start(). Fair enough. So if we make h3 an
>> unsigned int, the issue goes away, no?
>
> For the current version of clang, yes. However, if we're fixing this
> for good a char * pointer is the only guaranteed thing because it
> mirrors current use in printf.
>
All right. I guess I wasn't aware that non-printf like variadic
functions are now considered harmful or of the impending crusade against
them :)
But in the context of this patch, can we please use something less
invasive than changing all the arguments around? Promoting h3 to a bool
(if possible) or int/unsigned int would get my vote.
Regards,
-Denis
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-12 15:53 ` Denis Kenzior
0 siblings, 0 replies; 68+ messages in thread
From: Denis Kenzior @ 2018-10-12 15:53 UTC (permalink / raw)
To: James Bottomley, ndesaulniers, dhowells
Cc: natechancellor, ebiggers, Mimi Zohar, James Morris,
Serge E. Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
Hi James,
>> From the links provided in the patch it seems that one cannot pass
>> char/float/short to va_start(). Fair enough. So if we make h3 an
>> unsigned int, the issue goes away, no?
>
> For the current version of clang, yes. However, if we're fixing this
> for good a char * pointer is the only guaranteed thing because it
> mirrors current use in printf.
>
All right. I guess I wasn't aware that non-printf like variadic
functions are now considered harmful or of the impending crusade against
them :)
But in the context of this patch, can we please use something less
invasive than changing all the arguments around? Promoting h3 to a bool
(if possible) or int/unsigned int would get my vote.
Regards,
-Denis
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
2018-10-12 15:53 ` Denis Kenzior
@ 2018-10-12 16:01 ` James Bottomley
-1 siblings, 0 replies; 68+ messages in thread
From: James Bottomley @ 2018-10-12 16:01 UTC (permalink / raw)
To: Denis Kenzior, ndesaulniers, dhowells
Cc: natechancellor, ebiggers, Mimi Zohar, James Morris,
Serge E. Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
On Fri, 2018-10-12 at 10:53 -0500, Denis Kenzior wrote:
> Hi James,
>
> > > From the links provided in the patch it seems that one cannot
> > > pass char/float/short to va_start(). Fair enough. So if we make
> > > h3 an unsigned int, the issue goes away, no?
> >
> > For the current version of clang, yes. However, if we're fixing
> > this for good a char * pointer is the only guaranteed thing because
> > it mirrors current use in printf.
> >
>
> All right. I guess I wasn't aware that non-printf like variadic
> functions are now considered harmful or of the impending crusade
> against them :)
It's not, it's just a maintainer issue: The original problem is because
we coded for gcc specifically; it doesn't complain and does the right
thing, so everyone was happy. Now Clang comes along and is unhappy
with this, so the question a good maintainer should ask is "how do I
fix this so it never comes back again?", not "what's the easiest
bandaid to get both Clang and gcc to work?" because the latter is how
we got here in the first place.
James
> But in the context of this patch, can we please use something less
> invasive than changing all the arguments around? Promoting h3 to a
> bool (if possible) or int/unsigned int would get my vote.
>
> Regards,
> -Denis
>
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-12 16:01 ` James Bottomley
0 siblings, 0 replies; 68+ messages in thread
From: James Bottomley @ 2018-10-12 16:01 UTC (permalink / raw)
To: Denis Kenzior, ndesaulniers, dhowells
Cc: natechancellor, ebiggers, Mimi Zohar, James Morris,
Serge E. Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
On Fri, 2018-10-12 at 10:53 -0500, Denis Kenzior wrote:
> Hi James,
>
> > > From the links provided in the patch it seems that one cannot
> > > pass char/float/short to va_start(). Fair enough. So if we make
> > > h3 an unsigned int, the issue goes away, no?
> >
> > For the current version of clang, yes. However, if we're fixing
> > this for good a char * pointer is the only guaranteed thing because
> > it mirrors current use in printf.
> >
>
> All right. I guess I wasn't aware that non-printf like variadic
> functions are now considered harmful or of the impending crusade
> against them :)
It's not, it's just a maintainer issue: The original problem is because
we coded for gcc specifically; it doesn't complain and does the right
thing, so everyone was happy. Now Clang comes along and is unhappy
with this, so the question a good maintainer should ask is "how do I
fix this so it never comes back again?", not "what's the easiest
bandaid to get both Clang and gcc to work?" because the latter is how
we got here in the first place.
James
> But in the context of this patch, can we please use something less
> invasive than changing all the arguments around? Promoting h3 to a
> bool (if possible) or int/unsigned int would get my vote.
>
> Regards,
> -Denis
>
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
2018-10-12 16:01 ` James Bottomley
@ 2018-10-12 17:14 ` Nick Desaulniers
-1 siblings, 0 replies; 68+ messages in thread
From: Nick Desaulniers @ 2018-10-12 17:14 UTC (permalink / raw)
To: jejb
Cc: denkenz, dhowells, Nathan Chancellor, Eric Biggers, zohar,
jmorris, serge, linux-integrity, keyrings, linux-security-module,
LKML
On Fri, Oct 12, 2018 at 9:01 AM James Bottomley <jejb@linux.ibm.com> wrote:
>
> On Fri, 2018-10-12 at 10:53 -0500, Denis Kenzior wrote:
> > Hi James,
> >
> > > > From the links provided in the patch it seems that one cannot
> > > > pass char/float/short to va_start(). Fair enough. So if we make
> > > > h3 an unsigned int, the issue goes away, no?
> > >
> > > For the current version of clang, yes. However, if we're fixing
> > > this for good a char * pointer is the only guaranteed thing because
> > > it mirrors current use in printf.
> > >
> >
> > All right. I guess I wasn't aware that non-printf like variadic
> > functions are now considered harmful or of the impending crusade
> > against them :)
>
> It's not, it's just a maintainer issue: The original problem is because
> we coded for gcc specifically; it doesn't complain and does the right
> thing, so everyone was happy. Now Clang comes along and is unhappy
> with this, so the question a good maintainer should ask is "how do I
> fix this so it never comes back again?", not "what's the easiest
> bandaid to get both Clang and gcc to work?" because the latter is how
> we got here in the first place.
Clang is simply pointing out that this is explicitly undefined
behavior by the C90 spec. As such, not only may the implementation be
different between compilers, but it is free to change between versions
of the same compiler (I haven't witnessed such a case yet in my
limited career, but I would never say never when it comes to relying
on undefined behavior).
>
> James
>
> > But in the context of this patch, can we please use something less
> > invasive than changing all the arguments around? Promoting h3 to a
> > bool (if possible) or int/unsigned int would get my vote.
> >
> > Regards,
> > -Denis
> >
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-12 17:14 ` Nick Desaulniers
0 siblings, 0 replies; 68+ messages in thread
From: Nick Desaulniers @ 2018-10-12 17:14 UTC (permalink / raw)
To: jejb
Cc: denkenz, dhowells, Nathan Chancellor, Eric Biggers, zohar,
jmorris, serge, linux-integrity, keyrings, linux-security-module,
LKML
On Fri, Oct 12, 2018 at 9:01 AM James Bottomley <jejb@linux.ibm.com> wrote:
>
> On Fri, 2018-10-12 at 10:53 -0500, Denis Kenzior wrote:
> > Hi James,
> >
> > > > From the links provided in the patch it seems that one cannot
> > > > pass char/float/short to va_start(). Fair enough. So if we make
> > > > h3 an unsigned int, the issue goes away, no?
> > >
> > > For the current version of clang, yes. However, if we're fixing
> > > this for good a char * pointer is the only guaranteed thing because
> > > it mirrors current use in printf.
> > >
> >
> > All right. I guess I wasn't aware that non-printf like variadic
> > functions are now considered harmful or of the impending crusade
> > against them :)
>
> It's not, it's just a maintainer issue: The original problem is because
> we coded for gcc specifically; it doesn't complain and does the right
> thing, so everyone was happy. Now Clang comes along and is unhappy
> with this, so the question a good maintainer should ask is "how do I
> fix this so it never comes back again?", not "what's the easiest
> bandaid to get both Clang and gcc to work?" because the latter is how
> we got here in the first place.
Clang is simply pointing out that this is explicitly undefined
behavior by the C90 spec. As such, not only may the implementation be
different between compilers, but it is free to change between versions
of the same compiler (I haven't witnessed such a case yet in my
limited career, but I would never say never when it comes to relying
on undefined behavior).
>
> James
>
> > But in the context of this patch, can we please use something less
> > invasive than changing all the arguments around? Promoting h3 to a
> > bool (if possible) or int/unsigned int would get my vote.
> >
> > Regards,
> > -Denis
> >
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
2018-10-12 15:13 ` Denis Kenzior
@ 2018-10-12 15:25 ` James Bottomley
-1 siblings, 0 replies; 68+ messages in thread
From: James Bottomley @ 2018-10-12 15:25 UTC (permalink / raw)
To: Denis Kenzior, ndesaulniers, dhowells
Cc: natechancellor, ebiggers, Mimi Zohar, James Morris,
Serge E. Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
On Fri, 2018-10-12 at 10:13 -0500, Denis Kenzior wrote:
> Hi James,
>
> > > So can't we simply use 'bool' or uint32 as the type for h3
> > > instead
> > > of re-ordering everything
> >
> > The problem is the standard is ambiguious. The only thing that's
> > guaranteed to work for all time is a char *. If you want to keep
> > the
> > order, what I'd suggest is inserting a dummy pointer argument which
> > is
> > always expected to be NULL between the h3 and the varargs.
> https://patchwork.kernel.org/patch/10274411/
> So maybe I'm misunderstanding something, but the issue seems to be
> that
> unsigned char is promoted to 'unsigned char *' by Clang and probably
> unsigned int or int by gcc.
>
> So instead of having unsigned char h3, can't we simply have bool h3
> or unsigned int h3?
Or actually, we fix this like I did for tpm2 in an unmerged patch and
compute the hmac over the constructed buffer not using varargs:
https://patchwork.kernel.org/patch/10274411/
James
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-12 15:25 ` James Bottomley
0 siblings, 0 replies; 68+ messages in thread
From: James Bottomley @ 2018-10-12 15:25 UTC (permalink / raw)
To: Denis Kenzior, ndesaulniers, dhowells
Cc: natechancellor, ebiggers, Mimi Zohar, James Morris,
Serge E. Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
On Fri, 2018-10-12 at 10:13 -0500, Denis Kenzior wrote:
> Hi James,
>
> > > So can't we simply use 'bool' or uint32 as the type for h3
> > > instead
> > > of re-ordering everything
> >
> > The problem is the standard is ambiguious. The only thing that's
> > guaranteed to work for all time is a char *. If you want to keep
> > the
> > order, what I'd suggest is inserting a dummy pointer argument which
> > is
> > always expected to be NULL between the h3 and the varargs.
> https://patchwork.kernel.org/patch/10274411/
> So maybe I'm misunderstanding something, but the issue seems to be
> that
> unsigned char is promoted to 'unsigned char *' by Clang and probably
> unsigned int or int by gcc.
>
> So instead of having unsigned char h3, can't we simply have bool h3
> or unsigned int h3?
Or actually, we fix this like I did for tpm2 in an unmerged patch and
compute the hmac over the constructed buffer not using varargs:
https://patchwork.kernel.org/patch/10274411/
James
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
2018-10-12 15:13 ` Denis Kenzior
@ 2018-10-12 17:05 ` Nick Desaulniers
-1 siblings, 0 replies; 68+ messages in thread
From: Nick Desaulniers @ 2018-10-12 17:05 UTC (permalink / raw)
To: denkenz
Cc: jejb, dhowells, Nathan Chancellor, Eric Biggers, zohar, jmorris,
serge, linux-integrity, keyrings, linux-security-module, LKML
On Fri, Oct 12, 2018 at 8:14 AM Denis Kenzior <denkenz@gmail.com> wrote:
>
> Hi James,
>
> >> So can't we simply use 'bool' or uint32 as the type for h3 instead
> >> of re-ordering everything
> >
> > The problem is the standard is ambiguious. The only thing that's
> > guaranteed to work for all time is a char *. If you want to keep the
> > order, what I'd suggest is inserting a dummy pointer argument which is
> > always expected to be NULL between the h3 and the varargs.
>
> So maybe I'm misunderstanding something, but the issue seems to be that
> unsigned char is promoted to 'unsigned char *' by Clang and probably
> unsigned int or int by gcc.
No. This is extremely well defined behavior in C. In C, integral
types are NEVER promoted to pointer to integer types, only to larger
integral types through rules more complicated than the correct flags
to pass to `tar`.
https://xkcd.com/1168/
>
> So instead of having unsigned char h3, can't we simply have bool h3 or
> unsigned int h3?
int is the default argument promotion. Proposed:
https://github.com/ClangBuiltLinux/linux/issues/41#issuecomment-428365339
>
> Regards,
> -Denis
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-12 17:05 ` Nick Desaulniers
0 siblings, 0 replies; 68+ messages in thread
From: Nick Desaulniers @ 2018-10-12 17:05 UTC (permalink / raw)
To: denkenz
Cc: jejb, dhowells, Nathan Chancellor, Eric Biggers, zohar, jmorris,
serge, linux-integrity, keyrings, linux-security-module, LKML
On Fri, Oct 12, 2018 at 8:14 AM Denis Kenzior <denkenz@gmail.com> wrote:
>
> Hi James,
>
> >> So can't we simply use 'bool' or uint32 as the type for h3 instead
> >> of re-ordering everything
> >
> > The problem is the standard is ambiguious. The only thing that's
> > guaranteed to work for all time is a char *. If you want to keep the
> > order, what I'd suggest is inserting a dummy pointer argument which is
> > always expected to be NULL between the h3 and the varargs.
>
> So maybe I'm misunderstanding something, but the issue seems to be that
> unsigned char is promoted to 'unsigned char *' by Clang and probably
> unsigned int or int by gcc.
No. This is extremely well defined behavior in C. In C, integral
types are NEVER promoted to pointer to integer types, only to larger
integral types through rules more complicated than the correct flags
to pass to `tar`.
https://xkcd.com/1168/
>
> So instead of having unsigned char h3, can't we simply have bool h3 or
> unsigned int h3?
int is the default argument promotion. Proposed:
https://github.com/ClangBuiltLinux/linux/issues/41#issuecomment-428365339
>
> Regards,
> -Denis
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
2018-10-12 17:05 ` Nick Desaulniers
@ 2018-10-12 17:17 ` Nick Desaulniers
-1 siblings, 0 replies; 68+ messages in thread
From: Nick Desaulniers @ 2018-10-12 17:17 UTC (permalink / raw)
To: denkenz
Cc: jejb, dhowells, Nathan Chancellor, Eric Biggers, zohar, jmorris,
serge, linux-integrity, keyrings, linux-security-module, LKML
On Fri, Oct 12, 2018 at 10:05 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Fri, Oct 12, 2018 at 8:14 AM Denis Kenzior <denkenz@gmail.com> wrote:
> >
> > Hi James,
> >
> > >> So can't we simply use 'bool' or uint32 as the type for h3 instead
> > >> of re-ordering everything
> > >
> > > The problem is the standard is ambiguious. The only thing that's
> > > guaranteed to work for all time is a char *. If you want to keep the
> > > order, what I'd suggest is inserting a dummy pointer argument which is
> > > always expected to be NULL between the h3 and the varargs.
> >
> > So maybe I'm misunderstanding something, but the issue seems to be that
> > unsigned char is promoted to 'unsigned char *' by Clang and probably
> > unsigned int or int by gcc.
>
> No. This is extremely well defined behavior in C. In C, integral
> types are NEVER promoted to pointer to integer types, only to larger
> integral types through rules more complicated than the correct flags
> to pass to `tar`.
> https://xkcd.com/1168/
And may have their signedness converted.
https://wiki.sei.cmu.edu/confluence/display/c/INT02-C.+Understand+integer+conversion+rules
is the reference I use, though I always feel like there's quite a bit
of mental gymnastics involved interpreting it.
>
> >
> > So instead of having unsigned char h3, can't we simply have bool h3 or
> > unsigned int h3?
>
> int is the default argument promotion. Proposed:
> https://github.com/ClangBuiltLinux/linux/issues/41#issuecomment-428365339
>
> >
> > Regards,
> > -Denis
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-12 17:17 ` Nick Desaulniers
0 siblings, 0 replies; 68+ messages in thread
From: Nick Desaulniers @ 2018-10-12 17:17 UTC (permalink / raw)
To: denkenz
Cc: jejb, dhowells, Nathan Chancellor, Eric Biggers, zohar, jmorris,
serge, linux-integrity, keyrings, linux-security-module, LKML
On Fri, Oct 12, 2018 at 10:05 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Fri, Oct 12, 2018 at 8:14 AM Denis Kenzior <denkenz@gmail.com> wrote:
> >
> > Hi James,
> >
> > >> So can't we simply use 'bool' or uint32 as the type for h3 instead
> > >> of re-ordering everything
> > >
> > > The problem is the standard is ambiguious. The only thing that's
> > > guaranteed to work for all time is a char *. If you want to keep the
> > > order, what I'd suggest is inserting a dummy pointer argument which is
> > > always expected to be NULL between the h3 and the varargs.
> >
> > So maybe I'm misunderstanding something, but the issue seems to be that
> > unsigned char is promoted to 'unsigned char *' by Clang and probably
> > unsigned int or int by gcc.
>
> No. This is extremely well defined behavior in C. In C, integral
> types are NEVER promoted to pointer to integer types, only to larger
> integral types through rules more complicated than the correct flags
> to pass to `tar`.
> https://xkcd.com/1168/
And may have their signedness converted.
https://wiki.sei.cmu.edu/confluence/display/c/INT02-C.+Understand+integer+conversion+rules
is the reference I use, though I always feel like there's quite a bit
of mental gymnastics involved interpreting it.
>
> >
> > So instead of having unsigned char h3, can't we simply have bool h3 or
> > unsigned int h3?
>
> int is the default argument promotion. Proposed:
> https://github.com/ClangBuiltLinux/linux/issues/41#issuecomment-428365339
>
> >
> > Regards,
> > -Denis
> >
>
>
> --
> Thanks,
> ~Nick Desaulniers
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
2018-10-12 17:05 ` Nick Desaulniers
@ 2018-10-12 17:27 ` Denis Kenzior
-1 siblings, 0 replies; 68+ messages in thread
From: Denis Kenzior @ 2018-10-12 17:27 UTC (permalink / raw)
To: Nick Desaulniers
Cc: jejb, dhowells, Nathan Chancellor, Eric Biggers, zohar, jmorris,
serge, linux-integrity, keyrings, linux-security-module, LKML
Hi Nick,
>> So maybe I'm misunderstanding something, but the issue seems to be that
>> unsigned char is promoted to 'unsigned char *' by Clang and probably
>> unsigned int or int by gcc.
>
> No. This is extremely well defined behavior in C. In C, integral
> types are NEVER promoted to pointer to integer types, only to larger
> integral types through rules more complicated than the correct flags
> to pass to `tar`.
> https://xkcd.com/1168/
>
Ah right. Thanks for the correction. So looks like bool won't work for
the same reasons. But unsigned int should work right? But then again
this is a boolean value and if we want to be paranoid we can simply
tweak the 'c = h3' assignment to be something like:
c = !!h3;
So in the end, I'm happy with int or unsigned int.
Regards,
-Denis
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-12 17:27 ` Denis Kenzior
0 siblings, 0 replies; 68+ messages in thread
From: Denis Kenzior @ 2018-10-12 17:27 UTC (permalink / raw)
To: Nick Desaulniers
Cc: jejb, dhowells, Nathan Chancellor, Eric Biggers, zohar, jmorris,
serge, linux-integrity, keyrings, linux-security-module, LKML
Hi Nick,
>> So maybe I'm misunderstanding something, but the issue seems to be that
>> unsigned char is promoted to 'unsigned char *' by Clang and probably
>> unsigned int or int by gcc.
>
> No. This is extremely well defined behavior in C. In C, integral
> types are NEVER promoted to pointer to integer types, only to larger
> integral types through rules more complicated than the correct flags
> to pass to `tar`.
> https://xkcd.com/1168/
>
Ah right. Thanks for the correction. So looks like bool won't work for
the same reasons. But unsigned int should work right? But then again
this is a boolean value and if we want to be paranoid we can simply
tweak the 'c = h3' assignment to be something like:
c = !!h3;
So in the end, I'm happy with int or unsigned int.
Regards,
-Denis
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
2018-10-12 17:27 ` Denis Kenzior
@ 2018-10-12 18:39 ` Nick Desaulniers
-1 siblings, 0 replies; 68+ messages in thread
From: Nick Desaulniers @ 2018-10-12 18:39 UTC (permalink / raw)
To: denkenz
Cc: jejb, dhowells, Nathan Chancellor, Eric Biggers, zohar, jmorris,
serge, linux-integrity, keyrings, linux-security-module, LKML
On Fri, Oct 12, 2018 at 10:27 AM Denis Kenzior <denkenz@gmail.com> wrote:
>
> Hi Nick,
>
> >> So maybe I'm misunderstanding something, but the issue seems to be that
> >> unsigned char is promoted to 'unsigned char *' by Clang and probably
> >> unsigned int or int by gcc.
> >
> > No. This is extremely well defined behavior in C. In C, integral
> > types are NEVER promoted to pointer to integer types, only to larger
> > integral types through rules more complicated than the correct flags
> > to pass to `tar`.
> > https://xkcd.com/1168/
> >
>
> Ah right. Thanks for the correction. So looks like bool won't work for
> the same reasons. But unsigned int should work right? But then again
> this is a boolean value and if we want to be paranoid we can simply
> tweak the 'c = h3' assignment to be something like:
>
> c = !!h3;
>
> So in the end, I'm happy with int or unsigned int.
Thanks for the feedback. I'll wait wait to see if James is also cool
with that approach, and if so, send a v2 based on the next-keys branch
in the security tree as per Nathan, with yours and his Suggested-by
tags.
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-12 18:39 ` Nick Desaulniers
0 siblings, 0 replies; 68+ messages in thread
From: Nick Desaulniers @ 2018-10-12 18:39 UTC (permalink / raw)
To: denkenz
Cc: jejb, dhowells, Nathan Chancellor, Eric Biggers, zohar, jmorris,
serge, linux-integrity, keyrings, linux-security-module, LKML
On Fri, Oct 12, 2018 at 10:27 AM Denis Kenzior <denkenz@gmail.com> wrote:
>
> Hi Nick,
>
> >> So maybe I'm misunderstanding something, but the issue seems to be that
> >> unsigned char is promoted to 'unsigned char *' by Clang and probably
> >> unsigned int or int by gcc.
> >
> > No. This is extremely well defined behavior in C. In C, integral
> > types are NEVER promoted to pointer to integer types, only to larger
> > integral types through rules more complicated than the correct flags
> > to pass to `tar`.
> > https://xkcd.com/1168/
> >
>
> Ah right. Thanks for the correction. So looks like bool won't work for
> the same reasons. But unsigned int should work right? But then again
> this is a boolean value and if we want to be paranoid we can simply
> tweak the 'c = h3' assignment to be something like:
>
> c = !!h3;
>
> So in the end, I'm happy with int or unsigned int.
Thanks for the feedback. I'll wait wait to see if James is also cool
with that approach, and if so, send a v2 based on the next-keys branch
in the security tree as per Nathan, with yours and his Suggested-by
tags.
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
2018-10-12 12:29 ` Denis Kenzior
@ 2018-10-12 17:02 ` Nick Desaulniers
-1 siblings, 0 replies; 68+ messages in thread
From: Nick Desaulniers @ 2018-10-12 17:02 UTC (permalink / raw)
To: denkenz
Cc: James E.J. Bottomley, dhowells, Nathan Chancellor, Eric Biggers,
zohar, jmorris, serge, linux-integrity, keyrings,
linux-security-module, LKML
On Fri, Oct 12, 2018 at 5:29 AM Denis Kenzior <denkenz@gmail.com> wrote:
>
> Hi Nick,
>
> > @@ -123,7 +123,7 @@ static int TSS_rawhmac(unsigned char *digest, const unsigned char *key,
> > */
> > static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
> > unsigned int keylen, unsigned char *h1,
> > - unsigned char *h2, unsigned char h3, ...)
> > + unsigned char h2, unsigned char *h3, ...)
> > {
> > unsigned char paramdigest[SHA1_DIGEST_SIZE];
> > struct sdesc *sdesc;
>
> So my concern here is that this actually breaks the natural argument
> order compared to what the specification uses. This in turn requires
> one to perform some mental gymnastics and I'm not sure that this is such
> a good idea.
Thanks for the review.
> Refer to
> https://trustedcomputinggroup.org/wp-content/uploads/TPM-Main-Part-3-Commands_v1.2_rev116_01032011.pdf
> for details.
Can you cite the relevant section?
>
> Note that H3 is really the 'continueAuthSession' variable which is a
> bool. In the above specification BOOL has a size of 1, and TSS_authhmac
> already assigns a h3 to 'c' which is used for the actual hashing.
>
> So can't we simply use 'bool' or uint32 as the type for h3 instead of
> re-ordering everything?
int was exactly what I originally proposed:
https://github.com/ClangBuiltLinux/linux/issues/41#issuecomment-428365339.
If that works for you and the maintainers, I can send that in patch
form.
>
> Regards,
> -Denis
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-12 17:02 ` Nick Desaulniers
0 siblings, 0 replies; 68+ messages in thread
From: Nick Desaulniers @ 2018-10-12 17:02 UTC (permalink / raw)
To: denkenz
Cc: James E.J. Bottomley, dhowells, Nathan Chancellor, Eric Biggers,
zohar, jmorris, serge, linux-integrity, keyrings,
linux-security-module, LKML
On Fri, Oct 12, 2018 at 5:29 AM Denis Kenzior <denkenz@gmail.com> wrote:
>
> Hi Nick,
>
> > @@ -123,7 +123,7 @@ static int TSS_rawhmac(unsigned char *digest, const unsigned char *key,
> > */
> > static int TSS_authhmac(unsigned char *digest, const unsigned char *key,
> > unsigned int keylen, unsigned char *h1,
> > - unsigned char *h2, unsigned char h3, ...)
> > + unsigned char h2, unsigned char *h3, ...)
> > {
> > unsigned char paramdigest[SHA1_DIGEST_SIZE];
> > struct sdesc *sdesc;
>
> So my concern here is that this actually breaks the natural argument
> order compared to what the specification uses. This in turn requires
> one to perform some mental gymnastics and I'm not sure that this is such
> a good idea.
Thanks for the review.
> Refer to
> https://trustedcomputinggroup.org/wp-content/uploads/TPM-Main-Part-3-Commands_v1.2_rev116_01032011.pdf
> for details.
Can you cite the relevant section?
>
> Note that H3 is really the 'continueAuthSession' variable which is a
> bool. In the above specification BOOL has a size of 1, and TSS_authhmac
> already assigns a h3 to 'c' which is used for the actual hashing.
>
> So can't we simply use 'bool' or uint32 as the type for h3 instead of
> re-ordering everything?
int was exactly what I originally proposed:
https://github.com/ClangBuiltLinux/linux/issues/41#issuecomment-428365339.
If that works for you and the maintainers, I can send that in patch
form.
>
> Regards,
> -Denis
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
2018-10-12 17:02 ` Nick Desaulniers
@ 2018-10-12 17:15 ` Denis Kenzior
-1 siblings, 0 replies; 68+ messages in thread
From: Denis Kenzior @ 2018-10-12 17:15 UTC (permalink / raw)
To: Nick Desaulniers
Cc: James E.J. Bottomley, dhowells, Nathan Chancellor, Eric Biggers,
zohar, jmorris, serge, linux-integrity, keyrings,
linux-security-module, LKML
Hi Nick,
>> Refer to
>> https://trustedcomputinggroup.org/wp-content/uploads/TPM-Main-Part-3-Commands_v1.2_rev116_01032011.pdf
>> for details.
>
> Can you cite the relevant section?
>
Just pick any section that describes a TPM command. I randomly used
Section 10.3 for TPM Unbind. See the 'Incoming operands and sizes' table.
Regards,
-Denis
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-12 17:15 ` Denis Kenzior
0 siblings, 0 replies; 68+ messages in thread
From: Denis Kenzior @ 2018-10-12 17:15 UTC (permalink / raw)
To: Nick Desaulniers
Cc: James E.J. Bottomley, dhowells, Nathan Chancellor, Eric Biggers,
zohar, jmorris, serge, linux-integrity, keyrings,
linux-security-module, LKML
Hi Nick,
>> Refer to
>> https://trustedcomputinggroup.org/wp-content/uploads/TPM-Main-Part-3-Commands_v1.2_rev116_01032011.pdf
>> for details.
>
> Can you cite the relevant section?
>
Just pick any section that describes a TPM command. I randomly used
Section 10.3 for TPM Unbind. See the 'Incoming operands and sizes' table.
Regards,
-Denis
^ permalink raw reply [flat|nested] 68+ messages in thread
* RE: [PATCH] KEYS: trusted: fix -Wvarags warning
2018-10-11 20:31 ` ndesaulniers
(?)
@ 2018-10-15 9:26 ` David Laight
-1 siblings, 0 replies; 68+ messages in thread
From: David Laight @ 2018-10-15 9:26 UTC (permalink / raw)
To: 'ndesaulniers@google.com', jejb, dhowells
Cc: natechancellor, ebiggers, Mimi Zohar, James Morris,
Serge E. Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
RnJvbTogbmRlc2F1bG5pZXJzQGdvb2dsZS5jb20NCj4gU2VudDogMTEgT2N0b2JlciAyMDE4IDIx
OjMxDQouLi4NCj4gYnkgc3dhcHBpbmcgaDIgYW5kIGgzLg0KPiANCj4gc2VjdXJpdHkva2V5cy90
cnVzdGVkLmM6MTQ2OjE3OiB3YXJuaW5nOiBwYXNzaW5nIGFuIG9iamVjdCB0aGF0DQo+IHVuZGVy
Z29lcyBkZWZhdWx0DQo+ICAgICAgIGFyZ3VtZW50IHByb21vdGlvbiB0byAndmFfc3RhcnQnIGhh
cyB1bmRlZmluZWQgYmVoYXZpb3IgWy1XdmFyYXJnc10NCj4gICAgICAgICB2YV9zdGFydChhcmdw
LCBoMyk7DQo+ICAgICAgICAgICAgICAgICAgICAgICAgXg0KPiBzZWN1cml0eS9rZXlzL3RydXN0
ZWQuYzoxMjY6Mzc6IG5vdGU6IHBhcmFtZXRlciBvZiB0eXBlICd1bnNpZ25lZA0KPiBjaGFyJyBp
cyBkZWNsYXJlZCBoZXJlDQo+IHVuc2lnbmVkIGNoYXIgKmgyLCB1bnNpZ25lZCBjaGFyIGgzLCAu
Li4pDQo+ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBeDQo+IFNwZWNpZmljYWxseSwg
aXQgc2VlbXMgdGhhdCBib3RoIHRoZSBDOTAgKDQuOC4xLjEpIGFuZCBDMTEgKDcuMTYuMS40KQ0K
PiBzdGFuZGFyZHMgZXhwbGljaXRseSBjYWxsIHRoaXMgb3V0IGFzIHVuZGVmaW5lZCBiZWhhdmlv
cjoNCg0KSSBndWVzcyB0aGF0IHByb2JsZW1zIGFyaXNlIHdoZW4gYWxsIHRoZSBhcmd1bWVudHMg
YXJlIHN0YWNrZWQNCmFuZCB2YV9zdGFydC92YV9hcmcgdXNlIG5haXZlIHBvaW50ZXIgbWFuaXB1
bGF0aW9uLg0KSW4gdGhhdCBjYXNlICZoMyBtaWdodCBiZSA0biszIGFsaWduZWQgc28gdmFfYXJn
KCkgd2lsbCBhY2Nlc3MNCm1pc2FsaWduZWQgc3RhY2sgbG9jYXRpb25zLg0KDQpJIGRvdWJ0IGFu
eSBtb2Rlcm4gY29tcGlsZXJzICh3aGVyZSB2YV9zdGFydCBhbmQgdmFfYXJnIGFyZSBidWlsdGlu
cykNCndpbGwgZ2V0IHRoaXMgJ3dyb25nJyBldmVuIHdoZW4gYWxsIGFyZ3VtZW50cyBhcmUgc3Rh
Y2tlZC4NCg0KU2VlbXMgY2xhbmcgaXMgYmVpbmcgb3ZlciBjYXV0aW91cy4NCg0KCURhdmlkDQoN
Ci0NClJlZ2lzdGVyZWQgQWRkcmVzcyBMYWtlc2lkZSwgQnJhbWxleSBSb2FkLCBNb3VudCBGYXJt
LCBNaWx0b24gS2V5bmVzLCBNSzEgMVBULCBVSw0KUmVnaXN0cmF0aW9uIE5vOiAxMzk3Mzg2IChX
YWxlcykNCg=
^ permalink raw reply [flat|nested] 68+ messages in thread
* RE: [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-15 9:26 ` David Laight
0 siblings, 0 replies; 68+ messages in thread
From: David Laight @ 2018-10-15 9:26 UTC (permalink / raw)
To: 'ndesaulniers@google.com', jejb, dhowells
Cc: natechancellor, ebiggers, Mimi Zohar, James Morris,
Serge E. Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
From: ndesaulniers@google.com
> Sent: 11 October 2018 21:31
...
> by swapping h2 and h3.
>
> security/keys/trusted.c:146:17: warning: passing an object that
> undergoes default
> argument promotion to 'va_start' has undefined behavior [-Wvarargs]
> va_start(argp, h3);
> ^
> security/keys/trusted.c:126:37: note: parameter of type 'unsigned
> char' is declared here
> unsigned char *h2, unsigned char h3, ...)
> ^
> Specifically, it seems that both the C90 (4.8.1.1) and C11 (7.16.1.4)
> standards explicitly call this out as undefined behavior:
I guess that problems arise when all the arguments are stacked
and va_start/va_arg use naive pointer manipulation.
In that case &h3 might be 4n+3 aligned so va_arg() will access
misaligned stack locations.
I doubt any modern compilers (where va_start and va_arg are builtins)
will get this 'wrong' even when all arguments are stacked.
Seems clang is being over cautious.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 68+ messages in thread
* RE: [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-15 9:26 ` David Laight
0 siblings, 0 replies; 68+ messages in thread
From: David Laight @ 2018-10-15 9:26 UTC (permalink / raw)
To: 'ndesaulniers@google.com', jejb, dhowells
Cc: natechancellor, ebiggers, Mimi Zohar, James Morris,
Serge E. Hallyn, linux-integrity, keyrings,
linux-security-module, linux-kernel
From: ndesaulniers@google.com
> Sent: 11 October 2018 21:31
...
> by swapping h2 and h3.
>
> security/keys/trusted.c:146:17: warning: passing an object that
> undergoes default
> argument promotion to 'va_start' has undefined behavior [-Wvarargs]
> va_start(argp, h3);
> ^
> security/keys/trusted.c:126:37: note: parameter of type 'unsigned
> char' is declared here
> unsigned char *h2, unsigned char h3, ...)
> ^
> Specifically, it seems that both the C90 (4.8.1.1) and C11 (7.16.1.4)
> standards explicitly call this out as undefined behavior:
I guess that problems arise when all the arguments are stacked
and va_start/va_arg use naive pointer manipulation.
In that case &h3 might be 4n+3 aligned so va_arg() will access
misaligned stack locations.
I doubt any modern compilers (where va_start and va_arg are builtins)
will get this 'wrong' even when all arguments are stacked.
Seems clang is being over cautious.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
2018-10-15 9:26 ` David Laight
@ 2018-10-15 21:53 ` Nick Desaulniers
-1 siblings, 0 replies; 68+ messages in thread
From: Nick Desaulniers @ 2018-10-15 21:53 UTC (permalink / raw)
To: David.Laight
Cc: James E.J. Bottomley, dhowells, Nathan Chancellor, Eric Biggers,
zohar, jmorris, serge, linux-integrity, keyrings,
linux-security-module, LKML
On Mon, Oct 15, 2018 at 2:26 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: ndesaulniers@google.com
> > Sent: 11 October 2018 21:31
> ...
> > by swapping h2 and h3.
> >
> > security/keys/trusted.c:146:17: warning: passing an object that
> > undergoes default
> > argument promotion to 'va_start' has undefined behavior [-Wvarargs]
> > va_start(argp, h3);
> > ^
> > security/keys/trusted.c:126:37: note: parameter of type 'unsigned
> > char' is declared here
> > unsigned char *h2, unsigned char h3, ...)
> > ^
> > Specifically, it seems that both the C90 (4.8.1.1) and C11 (7.16.1.4)
> > standards explicitly call this out as undefined behavior:
>
> I guess that problems arise when all the arguments are stacked
> and va_start/va_arg use naive pointer manipulation.
> In that case &h3 might be 4n+3 aligned so va_arg() will access
> misaligned stack locations.
>
> I doubt any modern compilers (where va_start and va_arg are builtins)
> will get this 'wrong' even when all arguments are stacked.
>
> Seems clang is being over cautious.
Yes; did you have feedback on the Denis' proposed fix, or another?
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-15 21:53 ` Nick Desaulniers
0 siblings, 0 replies; 68+ messages in thread
From: Nick Desaulniers @ 2018-10-15 21:53 UTC (permalink / raw)
To: David.Laight
Cc: James E.J. Bottomley, dhowells, Nathan Chancellor, Eric Biggers,
zohar, jmorris, serge, linux-integrity, keyrings,
linux-security-module, LKML
On Mon, Oct 15, 2018 at 2:26 AM David Laight <David.Laight@aculab.com> wrote:
>
> From: ndesaulniers@google.com
> > Sent: 11 October 2018 21:31
> ...
> > by swapping h2 and h3.
> >
> > security/keys/trusted.c:146:17: warning: passing an object that
> > undergoes default
> > argument promotion to 'va_start' has undefined behavior [-Wvarargs]
> > va_start(argp, h3);
> > ^
> > security/keys/trusted.c:126:37: note: parameter of type 'unsigned
> > char' is declared here
> > unsigned char *h2, unsigned char h3, ...)
> > ^
> > Specifically, it seems that both the C90 (4.8.1.1) and C11 (7.16.1.4)
> > standards explicitly call this out as undefined behavior:
>
> I guess that problems arise when all the arguments are stacked
> and va_start/va_arg use naive pointer manipulation.
> In that case &h3 might be 4n+3 aligned so va_arg() will access
> misaligned stack locations.
>
> I doubt any modern compilers (where va_start and va_arg are builtins)
> will get this 'wrong' even when all arguments are stacked.
>
> Seems clang is being over cautious.
Yes; did you have feedback on the Denis' proposed fix, or another?
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 68+ messages in thread
* RE: [PATCH] KEYS: trusted: fix -Wvarags warning
2018-10-15 21:53 ` Nick Desaulniers
(?)
@ 2018-10-16 8:13 ` David Laight
-1 siblings, 0 replies; 68+ messages in thread
From: David Laight @ 2018-10-16 8:13 UTC (permalink / raw)
To: 'Nick Desaulniers'
Cc: James E.J. Bottomley, dhowells, Nathan Chancellor, Eric Biggers,
zohar, jmorris, serge, linux-integrity, keyrings,
linux-security-module, LKML
RnJvbTogTmljayBEZXNhdWxuaWVycw0KPiBTZW50OiAxNSBPY3RvYmVyIDIwMTggMjI6NTQNCj4g
T24gTW9uLCBPY3QgMTUsIDIwMTggYXQgMjoyNiBBTSBEYXZpZCBMYWlnaHQgPERhdmlkLkxhaWdo
dEBhY3VsYWIuY29tPiB3cm90ZToNCj4gPg0KPiA+IEZyb206IG5kZXNhdWxuaWVyc0Bnb29nbGUu
Y29tDQo+ID4gPiBTZW50OiAxMSBPY3RvYmVyIDIwMTggMjE6MzENCj4gPiAuLi4NCj4gPiA+IGJ5
IHN3YXBwaW5nIGgyIGFuZCBoMy4NCj4gPiA+DQo+ID4gPiBzZWN1cml0eS9rZXlzL3RydXN0ZWQu
YzoxNDY6MTc6IHdhcm5pbmc6IHBhc3NpbmcgYW4gb2JqZWN0IHRoYXQNCj4gPiA+IHVuZGVyZ29l
cyBkZWZhdWx0DQo+ID4gPiAgICAgICBhcmd1bWVudCBwcm9tb3Rpb24gdG8gJ3ZhX3N0YXJ0JyBo
YXMgdW5kZWZpbmVkIGJlaGF2aW9yIFstV3ZhcmFyZ3NdDQo+ID4gPiAgICAgICAgIHZhX3N0YXJ0
KGFyZ3AsIGgzKTsNCj4gPiA+ICAgICAgICAgICAgICAgICAgICAgICAgXg0KPiA+ID4gc2VjdXJp
dHkva2V5cy90cnVzdGVkLmM6MTI2OjM3OiBub3RlOiBwYXJhbWV0ZXIgb2YgdHlwZSAndW5zaWdu
ZWQNCj4gPiA+IGNoYXInIGlzIGRlY2xhcmVkIGhlcmUNCj4gPiA+IHVuc2lnbmVkIGNoYXIgKmgy
LCB1bnNpZ25lZCBjaGFyIGgzLCAuLi4pDQo+ID4gPiAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgXg0KPiA+ID4gU3BlY2lmaWNhbGx5LCBpdCBzZWVtcyB0aGF0IGJvdGggdGhlIEM5MCAo
NC44LjEuMSkgYW5kIEMxMSAoNy4xNi4xLjQpDQo+ID4gPiBzdGFuZGFyZHMgZXhwbGljaXRseSBj
YWxsIHRoaXMgb3V0IGFzIHVuZGVmaW5lZCBiZWhhdmlvcjoNCj4gPg0KPiA+IEkgZ3Vlc3MgdGhh
dCBwcm9ibGVtcyBhcmlzZSB3aGVuIGFsbCB0aGUgYXJndW1lbnRzIGFyZSBzdGFja2VkDQo+ID4g
YW5kIHZhX3N0YXJ0L3ZhX2FyZyB1c2UgbmFpdmUgcG9pbnRlciBtYW5pcHVsYXRpb24uDQo+ID4g
SW4gdGhhdCBjYXNlICZoMyBtaWdodCBiZSA0biszIGFsaWduZWQgc28gdmFfYXJnKCkgd2lsbCBh
Y2Nlc3MNCj4gPiBtaXNhbGlnbmVkIHN0YWNrIGxvY2F0aW9ucy4NCj4gPg0KPiA+IEkgZG91YnQg
YW55IG1vZGVybiBjb21waWxlcnMgKHdoZXJlIHZhX3N0YXJ0IGFuZCB2YV9hcmcgYXJlIGJ1aWx0
aW5zKQ0KPiA+IHdpbGwgZ2V0IHRoaXMgJ3dyb25nJyBldmVuIHdoZW4gYWxsIGFyZ3VtZW50cyBh
cmUgc3RhY2tlZC4NCj4gPg0KPiA+IFNlZW1zIGNsYW5nIGlzIGJlaW5nIG92ZXIgY2F1dGlvdXMu
DQo+IA0KPiBZZXM7IGRpZCB5b3UgaGF2ZSBmZWVkYmFjayBvbiB0aGUgRGVuaXMnIHByb3Bvc2Vk
IGZpeCwgb3IgYW5vdGhlcj8NCg0KUGVyc29uYWxseSBJJ2QgYXZvaWQgY2hhciwgc2hvcnQgYW5k
IGJvb2wgZm9yIGJvdGggZnVuY3Rpb24NCmFyZ3VtZW50cyBhbmQgcmVzdWx0cyBzaW5jZSB0aGV5
IHR5cGljYWxseSByZXF1aXJlIGV4dHJhDQppbnN0cnVjdGlvbnMgZm9yIG1hc2tpbmcgdmFsdWVz
IChldGMpLg0KYm9vbCBpcyBwYXJ0aWN1bGFybHkgb2Jub3hpb3VzLg0KDQpJbiB0aGF0IGNhc2Ug
ZWl0aGVyIGludCBvciB1bnNpZ25lZCBpbnQgaXMgZ29vZC4NCg0KCURhdmlkDQoNCi0NClJlZ2lz
dGVyZWQgQWRkcmVzcyBMYWtlc2lkZSwgQnJhbWxleSBSb2FkLCBNb3VudCBGYXJtLCBNaWx0b24g
S2V5bmVzLCBNSzEgMVBULCBVSw0KUmVnaXN0cmF0aW9uIE5vOiAxMzk3Mzg2IChXYWxlcykNCg=
^ permalink raw reply [flat|nested] 68+ messages in thread
* RE: [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-16 8:13 ` David Laight
0 siblings, 0 replies; 68+ messages in thread
From: David Laight @ 2018-10-16 8:13 UTC (permalink / raw)
To: 'Nick Desaulniers'
Cc: James E.J. Bottomley, dhowells, Nathan Chancellor, Eric Biggers,
zohar, jmorris, serge, linux-integrity, keyrings,
linux-security-module, LKML
From: Nick Desaulniers
> Sent: 15 October 2018 22:54
> On Mon, Oct 15, 2018 at 2:26 AM David Laight <David.Laight@aculab.com> wrote:
> >
> > From: ndesaulniers@google.com
> > > Sent: 11 October 2018 21:31
> > ...
> > > by swapping h2 and h3.
> > >
> > > security/keys/trusted.c:146:17: warning: passing an object that
> > > undergoes default
> > > argument promotion to 'va_start' has undefined behavior [-Wvarargs]
> > > va_start(argp, h3);
> > > ^
> > > security/keys/trusted.c:126:37: note: parameter of type 'unsigned
> > > char' is declared here
> > > unsigned char *h2, unsigned char h3, ...)
> > > ^
> > > Specifically, it seems that both the C90 (4.8.1.1) and C11 (7.16.1.4)
> > > standards explicitly call this out as undefined behavior:
> >
> > I guess that problems arise when all the arguments are stacked
> > and va_start/va_arg use naive pointer manipulation.
> > In that case &h3 might be 4n+3 aligned so va_arg() will access
> > misaligned stack locations.
> >
> > I doubt any modern compilers (where va_start and va_arg are builtins)
> > will get this 'wrong' even when all arguments are stacked.
> >
> > Seems clang is being over cautious.
>
> Yes; did you have feedback on the Denis' proposed fix, or another?
Personally I'd avoid char, short and bool for both function
arguments and results since they typically require extra
instructions for masking values (etc).
bool is particularly obnoxious.
In that case either int or unsigned int is good.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 68+ messages in thread
* RE: [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-16 8:13 ` David Laight
0 siblings, 0 replies; 68+ messages in thread
From: David Laight @ 2018-10-16 8:13 UTC (permalink / raw)
To: 'Nick Desaulniers'
Cc: James E.J. Bottomley, dhowells, Nathan Chancellor, Eric Biggers,
zohar, jmorris, serge, linux-integrity, keyrings,
linux-security-module, LKML
From: Nick Desaulniers
> Sent: 15 October 2018 22:54
> On Mon, Oct 15, 2018 at 2:26 AM David Laight <David.Laight@aculab.com> wrote:
> >
> > From: ndesaulniers@google.com
> > > Sent: 11 October 2018 21:31
> > ...
> > > by swapping h2 and h3.
> > >
> > > security/keys/trusted.c:146:17: warning: passing an object that
> > > undergoes default
> > > argument promotion to 'va_start' has undefined behavior [-Wvarargs]
> > > va_start(argp, h3);
> > > ^
> > > security/keys/trusted.c:126:37: note: parameter of type 'unsigned
> > > char' is declared here
> > > unsigned char *h2, unsigned char h3, ...)
> > > ^
> > > Specifically, it seems that both the C90 (4.8.1.1) and C11 (7.16.1.4)
> > > standards explicitly call this out as undefined behavior:
> >
> > I guess that problems arise when all the arguments are stacked
> > and va_start/va_arg use naive pointer manipulation.
> > In that case &h3 might be 4n+3 aligned so va_arg() will access
> > misaligned stack locations.
> >
> > I doubt any modern compilers (where va_start and va_arg are builtins)
> > will get this 'wrong' even when all arguments are stacked.
> >
> > Seems clang is being over cautious.
>
> Yes; did you have feedback on the Denis' proposed fix, or another?
Personally I'd avoid char, short and bool for both function
arguments and results since they typically require extra
instructions for masking values (etc).
bool is particularly obnoxious.
In that case either int or unsigned int is good.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 68+ messages in thread
* [PATCH v2] [PATCH] KEYS: trusted: fix -Wvarags warning
2018-10-16 8:13 ` David Laight
@ 2018-10-22 23:43 ` ndesaulniers
-1 siblings, 0 replies; 68+ messages in thread
From: ndesaulniers @ 2018-10-22 23:43 UTC (permalink / raw)
To: dhowells, jejb
Cc: natechancellor, David.Laight, denkenz, Nick Desaulniers,
Jarkko Sakkinen, Mimi Zohar, James Morris, Serge E. Hallyn,
keyrings, linux-kernel, linux-integrity, linux-security-module
Fixes the warning reported by Clang:
security/keys/trusted.c:146:17: warning: passing an object that
undergoes default
argument promotion to 'va_start' has undefined behavior [-Wvarargs]
va_start(argp, h3);
^
security/keys/trusted.c:126:37: note: parameter of type 'unsigned
char' is declared here
unsigned char *h2, unsigned char h3, ...)
^
Specifically, it seems that both the C90 (4.8.1.1) and C11 (7.16.1.4)
standards explicitly call this out as undefined behavior:
The parameter parmN is the identifier of the rightmost parameter in
the variable parameter list in the function definition (the one just
before the ...). If the parameter parmN is declared with ... or with a
type that is not compatible with the type that results after
application of the default argument promotions, the behavior is
undefined.
Link: https://github.com/ClangBuiltLinux/linux/issues/41
Link: https://www.eskimo.com/~scs/cclass/int/sx11c.html
Suggested-by: David Laight <David.Laight@aculab.com>
Suggested-by: Denis Kenzior <denkenz@gmail.com>
Suggested-by: James Bottomley <jejb@linux.vnet.ibm.com>
Suggested-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
v1 -> v2:
* Don't reorder args, just use default function promotion type
of unsigned int.
* Add !! boolean cast as per Denis in
https://lkml.org/lkml/2018/10/12/838.
* Tested with gcc-8 and clang-8.
include/keys/trusted.h | 2 +-
security/keys/trusted.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/keys/trusted.h b/include/keys/trusted.h
index adbcb6817826..0071298b9b28 100644
--- a/include/keys/trusted.h
+++ b/include/keys/trusted.h
@@ -38,7 +38,7 @@ enum {
int TSS_authhmac(unsigned char *digest, const unsigned char *key,
unsigned int keylen, unsigned char *h1,
- unsigned char *h2, unsigned char h3, ...);
+ unsigned char *h2, unsigned int h3, ...);
int TSS_checkhmac1(unsigned char *buffer,
const uint32_t command,
const unsigned char *ononce,
diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index ff6789365a12..335ce6d1cf6b 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -123,7 +123,7 @@ static int TSS_rawhmac(unsigned char *digest, const unsigned char *key,
*/
int TSS_authhmac(unsigned char *digest, const unsigned char *key,
unsigned int keylen, unsigned char *h1,
- unsigned char *h2, unsigned char h3, ...)
+ unsigned char *h2, unsigned int h3, ...)
{
unsigned char paramdigest[SHA1_DIGEST_SIZE];
struct sdesc *sdesc;
@@ -139,7 +139,7 @@ int TSS_authhmac(unsigned char *digest, const unsigned char *key,
return PTR_ERR(sdesc);
}
- c = h3;
+ c = !!h3;
ret = crypto_shash_init(&sdesc->shash);
if (ret < 0)
goto out;
--
2.19.1.568.g152ad8e336-goog
^ permalink raw reply related [flat|nested] 68+ messages in thread
* [PATCH v2] [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-22 23:43 ` ndesaulniers
0 siblings, 0 replies; 68+ messages in thread
From: ndesaulniers @ 2018-10-22 23:43 UTC (permalink / raw)
To: dhowells, jejb
Cc: natechancellor, David.Laight, denkenz, Nick Desaulniers,
Jarkko Sakkinen, Mimi Zohar, James Morris, Serge E. Hallyn,
keyrings, linux-kernel, linux-integrity, linux-security-module
Fixes the warning reported by Clang:
security/keys/trusted.c:146:17: warning: passing an object that
undergoes default
argument promotion to 'va_start' has undefined behavior [-Wvarargs]
va_start(argp, h3);
^
security/keys/trusted.c:126:37: note: parameter of type 'unsigned
char' is declared here
unsigned char *h2, unsigned char h3, ...)
^
Specifically, it seems that both the C90 (4.8.1.1) and C11 (7.16.1.4)
standards explicitly call this out as undefined behavior:
The parameter parmN is the identifier of the rightmost parameter in
the variable parameter list in the function definition (the one just
before the ...). If the parameter parmN is declared with ... or with a
type that is not compatible with the type that results after
application of the default argument promotions, the behavior is
undefined.
Link: https://github.com/ClangBuiltLinux/linux/issues/41
Link: https://www.eskimo.com/~scs/cclass/int/sx11c.html
Suggested-by: David Laight <David.Laight@aculab.com>
Suggested-by: Denis Kenzior <denkenz@gmail.com>
Suggested-by: James Bottomley <jejb@linux.vnet.ibm.com>
Suggested-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
v1 -> v2:
* Don't reorder args, just use default function promotion type
of unsigned int.
* Add !! boolean cast as per Denis in
https://lkml.org/lkml/2018/10/12/838.
* Tested with gcc-8 and clang-8.
include/keys/trusted.h | 2 +-
security/keys/trusted.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/keys/trusted.h b/include/keys/trusted.h
index adbcb6817826..0071298b9b28 100644
--- a/include/keys/trusted.h
+++ b/include/keys/trusted.h
@@ -38,7 +38,7 @@ enum {
int TSS_authhmac(unsigned char *digest, const unsigned char *key,
unsigned int keylen, unsigned char *h1,
- unsigned char *h2, unsigned char h3, ...);
+ unsigned char *h2, unsigned int h3, ...);
int TSS_checkhmac1(unsigned char *buffer,
const uint32_t command,
const unsigned char *ononce,
diff --git a/security/keys/trusted.c b/security/keys/trusted.c
index ff6789365a12..335ce6d1cf6b 100644
--- a/security/keys/trusted.c
+++ b/security/keys/trusted.c
@@ -123,7 +123,7 @@ static int TSS_rawhmac(unsigned char *digest, const unsigned char *key,
*/
int TSS_authhmac(unsigned char *digest, const unsigned char *key,
unsigned int keylen, unsigned char *h1,
- unsigned char *h2, unsigned char h3, ...)
+ unsigned char *h2, unsigned int h3, ...)
{
unsigned char paramdigest[SHA1_DIGEST_SIZE];
struct sdesc *sdesc;
@@ -139,7 +139,7 @@ int TSS_authhmac(unsigned char *digest, const unsigned char *key,
return PTR_ERR(sdesc);
}
- c = h3;
+ c = !!h3;
ret = crypto_shash_init(&sdesc->shash);
if (ret < 0)
goto out;
--
2.19.1.568.g152ad8e336-goog
^ permalink raw reply related [flat|nested] 68+ messages in thread
* Re: [PATCH v2] [PATCH] KEYS: trusted: fix -Wvarags warning
2018-10-22 23:43 ` ndesaulniers
@ 2018-10-23 0:00 ` Nathan Chancellor
-1 siblings, 0 replies; 68+ messages in thread
From: Nathan Chancellor @ 2018-10-23 0:00 UTC (permalink / raw)
To: ndesaulniers
Cc: dhowells, jejb, David.Laight, denkenz, Jarkko Sakkinen,
Mimi Zohar, James Morris, Serge E. Hallyn, keyrings,
linux-kernel, linux-integrity, linux-security-module
On Mon, Oct 22, 2018 at 04:43:57PM -0700, ndesaulniers@google.com wrote:
> Fixes the warning reported by Clang:
> security/keys/trusted.c:146:17: warning: passing an object that
> undergoes default
> argument promotion to 'va_start' has undefined behavior [-Wvarargs]
> va_start(argp, h3);
> ^
> security/keys/trusted.c:126:37: note: parameter of type 'unsigned
> char' is declared here
> unsigned char *h2, unsigned char h3, ...)
> ^
> Specifically, it seems that both the C90 (4.8.1.1) and C11 (7.16.1.4)
> standards explicitly call this out as undefined behavior:
>
> The parameter parmN is the identifier of the rightmost parameter in
> the variable parameter list in the function definition (the one just
> before the ...). If the parameter parmN is declared with ... or with a
> type that is not compatible with the type that results after
> application of the default argument promotions, the behavior is
> undefined.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/41
> Link: https://www.eskimo.com/~scs/cclass/int/sx11c.html
> Suggested-by: David Laight <David.Laight@aculab.com>
> Suggested-by: Denis Kenzior <denkenz@gmail.com>
> Suggested-by: James Bottomley <jejb@linux.vnet.ibm.com>
> Suggested-by: Nathan Chancellor <natechancellor@gmail.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
Tested-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
> v1 -> v2:
> * Don't reorder args, just use default function promotion type
> of unsigned int.
> * Add !! boolean cast as per Denis in
> https://lkml.org/lkml/2018/10/12/838.
> * Tested with gcc-8 and clang-8.
>
> include/keys/trusted.h | 2 +-
> security/keys/trusted.c | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/include/keys/trusted.h b/include/keys/trusted.h
> index adbcb6817826..0071298b9b28 100644
> --- a/include/keys/trusted.h
> +++ b/include/keys/trusted.h
> @@ -38,7 +38,7 @@ enum {
>
> int TSS_authhmac(unsigned char *digest, const unsigned char *key,
> unsigned int keylen, unsigned char *h1,
> - unsigned char *h2, unsigned char h3, ...);
> + unsigned char *h2, unsigned int h3, ...);
> int TSS_checkhmac1(unsigned char *buffer,
> const uint32_t command,
> const unsigned char *ononce,
> diff --git a/security/keys/trusted.c b/security/keys/trusted.c
> index ff6789365a12..335ce6d1cf6b 100644
> --- a/security/keys/trusted.c
> +++ b/security/keys/trusted.c
> @@ -123,7 +123,7 @@ static int TSS_rawhmac(unsigned char *digest, const unsigned char *key,
> */
> int TSS_authhmac(unsigned char *digest, const unsigned char *key,
> unsigned int keylen, unsigned char *h1,
> - unsigned char *h2, unsigned char h3, ...)
> + unsigned char *h2, unsigned int h3, ...)
> {
> unsigned char paramdigest[SHA1_DIGEST_SIZE];
> struct sdesc *sdesc;
> @@ -139,7 +139,7 @@ int TSS_authhmac(unsigned char *digest, const unsigned char *key,
> return PTR_ERR(sdesc);
> }
>
> - c = h3;
> + c = !!h3;
> ret = crypto_shash_init(&sdesc->shash);
> if (ret < 0)
> goto out;
> --
> 2.19.1.568.g152ad8e336-goog
>
Thank you for the fix!
Nathan
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2] [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-23 0:00 ` Nathan Chancellor
0 siblings, 0 replies; 68+ messages in thread
From: Nathan Chancellor @ 2018-10-23 0:00 UTC (permalink / raw)
To: ndesaulniers
Cc: dhowells, jejb, David.Laight, denkenz, Jarkko Sakkinen,
Mimi Zohar, James Morris, Serge E. Hallyn, keyrings,
linux-kernel, linux-integrity, linux-security-module
On Mon, Oct 22, 2018 at 04:43:57PM -0700, ndesaulniers@google.com wrote:
> Fixes the warning reported by Clang:
> security/keys/trusted.c:146:17: warning: passing an object that
> undergoes default
> argument promotion to 'va_start' has undefined behavior [-Wvarargs]
> va_start(argp, h3);
> ^
> security/keys/trusted.c:126:37: note: parameter of type 'unsigned
> char' is declared here
> unsigned char *h2, unsigned char h3, ...)
> ^
> Specifically, it seems that both the C90 (4.8.1.1) and C11 (7.16.1.4)
> standards explicitly call this out as undefined behavior:
>
> The parameter parmN is the identifier of the rightmost parameter in
> the variable parameter list in the function definition (the one just
> before the ...). If the parameter parmN is declared with ... or with a
> type that is not compatible with the type that results after
> application of the default argument promotions, the behavior is
> undefined.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/41
> Link: https://www.eskimo.com/~scs/cclass/int/sx11c.html
> Suggested-by: David Laight <David.Laight@aculab.com>
> Suggested-by: Denis Kenzior <denkenz@gmail.com>
> Suggested-by: James Bottomley <jejb@linux.vnet.ibm.com>
> Suggested-by: Nathan Chancellor <natechancellor@gmail.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
Reviewed-by: Nathan Chancellor <natechancellor@gmail.com>
Tested-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
> v1 -> v2:
> * Don't reorder args, just use default function promotion type
> of unsigned int.
> * Add !! boolean cast as per Denis in
> https://lkml.org/lkml/2018/10/12/838.
> * Tested with gcc-8 and clang-8.
>
> include/keys/trusted.h | 2 +-
> security/keys/trusted.c | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/include/keys/trusted.h b/include/keys/trusted.h
> index adbcb6817826..0071298b9b28 100644
> --- a/include/keys/trusted.h
> +++ b/include/keys/trusted.h
> @@ -38,7 +38,7 @@ enum {
>
> int TSS_authhmac(unsigned char *digest, const unsigned char *key,
> unsigned int keylen, unsigned char *h1,
> - unsigned char *h2, unsigned char h3, ...);
> + unsigned char *h2, unsigned int h3, ...);
> int TSS_checkhmac1(unsigned char *buffer,
> const uint32_t command,
> const unsigned char *ononce,
> diff --git a/security/keys/trusted.c b/security/keys/trusted.c
> index ff6789365a12..335ce6d1cf6b 100644
> --- a/security/keys/trusted.c
> +++ b/security/keys/trusted.c
> @@ -123,7 +123,7 @@ static int TSS_rawhmac(unsigned char *digest, const unsigned char *key,
> */
> int TSS_authhmac(unsigned char *digest, const unsigned char *key,
> unsigned int keylen, unsigned char *h1,
> - unsigned char *h2, unsigned char h3, ...)
> + unsigned char *h2, unsigned int h3, ...)
> {
> unsigned char paramdigest[SHA1_DIGEST_SIZE];
> struct sdesc *sdesc;
> @@ -139,7 +139,7 @@ int TSS_authhmac(unsigned char *digest, const unsigned char *key,
> return PTR_ERR(sdesc);
> }
>
> - c = h3;
> + c = !!h3;
> ret = crypto_shash_init(&sdesc->shash);
> if (ret < 0)
> goto out;
> --
> 2.19.1.568.g152ad8e336-goog
>
Thank you for the fix!
Nathan
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2] [PATCH] KEYS: trusted: fix -Wvarags warning
2018-10-22 23:43 ` ndesaulniers
@ 2018-10-24 8:36 ` Jarkko Sakkinen
-1 siblings, 0 replies; 68+ messages in thread
From: Jarkko Sakkinen @ 2018-10-24 8:36 UTC (permalink / raw)
To: Nick Desaulniers
Cc: dhowells, jejb, natechancellor, David.Laight, denkenz,
Jarkko Sakkinen, Mimi Zohar, James Morris, Serge E. Hallyn,
keyrings, linux-kernel, linux-integrity, linux-security-module
On Mon, 22 Oct 2018, ndesaulniers@google.com wrote:
> Fixes the warning reported by Clang:
> security/keys/trusted.c:146:17: warning: passing an object that
> undergoes default
> argument promotion to 'va_start' has undefined behavior [-Wvarargs]
> va_start(argp, h3);
> ^
> security/keys/trusted.c:126:37: note: parameter of type 'unsigned
> char' is declared here
> unsigned char *h2, unsigned char h3, ...)
> ^
> Specifically, it seems that both the C90 (4.8.1.1) and C11 (7.16.1.4)
> standards explicitly call this out as undefined behavior:
>
> The parameter parmN is the identifier of the rightmost parameter in
> the variable parameter list in the function definition (the one just
> before the ...). If the parameter parmN is declared with ... or with a
> type that is not compatible with the type that results after
> application of the default argument promotions, the behavior is
> undefined.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/41
> Link: https://www.eskimo.com/~scs/cclass/int/sx11c.html
> Suggested-by: David Laight <David.Laight@aculab.com>
> Suggested-by: Denis Kenzior <denkenz@gmail.com>
> Suggested-by: James Bottomley <jejb@linux.vnet.ibm.com>
> Suggested-by: Nathan Chancellor <natechancellor@gmail.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
/Jarkko
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2] [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-24 8:36 ` Jarkko Sakkinen
0 siblings, 0 replies; 68+ messages in thread
From: Jarkko Sakkinen @ 2018-10-24 8:36 UTC (permalink / raw)
To: Nick Desaulniers
Cc: dhowells, jejb, natechancellor, David.Laight, denkenz,
Jarkko Sakkinen, Mimi Zohar, James Morris, Serge E. Hallyn,
keyrings, linux-kernel, linux-integrity, linux-security-module
On Mon, 22 Oct 2018, ndesaulniers@google.com wrote:
> Fixes the warning reported by Clang:
> security/keys/trusted.c:146:17: warning: passing an object that
> undergoes default
> argument promotion to 'va_start' has undefined behavior [-Wvarargs]
> va_start(argp, h3);
> ^
> security/keys/trusted.c:126:37: note: parameter of type 'unsigned
> char' is declared here
> unsigned char *h2, unsigned char h3, ...)
> ^
> Specifically, it seems that both the C90 (4.8.1.1) and C11 (7.16.1.4)
> standards explicitly call this out as undefined behavior:
>
> The parameter parmN is the identifier of the rightmost parameter in
> the variable parameter list in the function definition (the one just
> before the ...). If the parameter parmN is declared with ... or with a
> type that is not compatible with the type that results after
> application of the default argument promotions, the behavior is
> undefined.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/41
> Link: https://www.eskimo.com/~scs/cclass/int/sx11c.html
> Suggested-by: David Laight <David.Laight@aculab.com>
> Suggested-by: Denis Kenzior <denkenz@gmail.com>
> Suggested-by: James Bottomley <jejb@linux.vnet.ibm.com>
> Suggested-by: Nathan Chancellor <natechancellor@gmail.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
/Jarkko
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2] [PATCH] KEYS: trusted: fix -Wvarags warning
2018-10-24 8:36 ` Jarkko Sakkinen
@ 2018-10-29 17:54 ` Nick Desaulniers
-1 siblings, 0 replies; 68+ messages in thread
From: Nick Desaulniers @ 2018-10-29 17:54 UTC (permalink / raw)
To: jarkko.sakkinen
Cc: dhowells, James E.J. Bottomley, Nathan Chancellor, David.Laight,
denkenz, zohar, jmorris, serge, keyrings, LKML, linux-integrity,
linux-security-module
On Wed, Oct 24, 2018 at 1:37 AM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Mon, 22 Oct 2018, ndesaulniers@google.com wrote:
> > Fixes the warning reported by Clang:
> > security/keys/trusted.c:146:17: warning: passing an object that
> > undergoes default
> > argument promotion to 'va_start' has undefined behavior [-Wvarargs]
> > va_start(argp, h3);
> > ^
> > security/keys/trusted.c:126:37: note: parameter of type 'unsigned
> > char' is declared here
> > unsigned char *h2, unsigned char h3, ...)
> > ^
> > Specifically, it seems that both the C90 (4.8.1.1) and C11 (7.16.1.4)
> > standards explicitly call this out as undefined behavior:
> >
> > The parameter parmN is the identifier of the rightmost parameter in
> > the variable parameter list in the function definition (the one just
> > before the ...). If the parameter parmN is declared with ... or with a
> > type that is not compatible with the type that results after
> > application of the default argument promotions, the behavior is
> > undefined.
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/41
> > Link: https://www.eskimo.com/~scs/cclass/int/sx11c.html
> > Suggested-by: David Laight <David.Laight@aculab.com>
> > Suggested-by: Denis Kenzior <denkenz@gmail.com>
> > Suggested-by: James Bottomley <jejb@linux.vnet.ibm.com>
> > Suggested-by: Nathan Chancellor <natechancellor@gmail.com>
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>
> /Jarkko
Bumping the maintainers if this isn't already picked up?
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2] [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2018-10-29 17:54 ` Nick Desaulniers
0 siblings, 0 replies; 68+ messages in thread
From: Nick Desaulniers @ 2018-10-29 17:54 UTC (permalink / raw)
To: jarkko.sakkinen
Cc: dhowells, James E.J. Bottomley, Nathan Chancellor, David.Laight,
denkenz, zohar, jmorris, serge, keyrings, LKML, linux-integrity,
linux-security-module
On Wed, Oct 24, 2018 at 1:37 AM Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
>
> On Mon, 22 Oct 2018, ndesaulniers@google.com wrote:
> > Fixes the warning reported by Clang:
> > security/keys/trusted.c:146:17: warning: passing an object that
> > undergoes default
> > argument promotion to 'va_start' has undefined behavior [-Wvarargs]
> > va_start(argp, h3);
> > ^
> > security/keys/trusted.c:126:37: note: parameter of type 'unsigned
> > char' is declared here
> > unsigned char *h2, unsigned char h3, ...)
> > ^
> > Specifically, it seems that both the C90 (4.8.1.1) and C11 (7.16.1.4)
> > standards explicitly call this out as undefined behavior:
> >
> > The parameter parmN is the identifier of the rightmost parameter in
> > the variable parameter list in the function definition (the one just
> > before the ...). If the parameter parmN is declared with ... or with a
> > type that is not compatible with the type that results after
> > application of the default argument promotions, the behavior is
> > undefined.
> >
> > Link: https://github.com/ClangBuiltLinux/linux/issues/41
> > Link: https://www.eskimo.com/~scs/cclass/int/sx11c.html
> > Suggested-by: David Laight <David.Laight@aculab.com>
> > Suggested-by: Denis Kenzior <denkenz@gmail.com>
> > Suggested-by: James Bottomley <jejb@linux.vnet.ibm.com>
> > Suggested-by: Nathan Chancellor <natechancellor@gmail.com>
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>
> /Jarkko
Bumping the maintainers if this isn't already picked up?
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2] [PATCH] KEYS: trusted: fix -Wvarags warning
2018-10-29 17:54 ` Nick Desaulniers
@ 2019-02-11 18:36 ` Nick Desaulniers
-1 siblings, 0 replies; 68+ messages in thread
From: Nick Desaulniers @ 2019-02-11 18:36 UTC (permalink / raw)
To: James E.J. Bottomley, jarkko.sakkinen, zohar
Cc: David Howells, Nathan Chancellor, David.Laight, denkenz, jmorris,
serge, keyrings, LKML, linux-integrity, linux-security-module
On Mon, Oct 29, 2018 at 10:54 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Wed, Oct 24, 2018 at 1:37 AM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Mon, 22 Oct 2018, ndesaulniers@google.com wrote:
> > > Fixes the warning reported by Clang:
> > > security/keys/trusted.c:146:17: warning: passing an object that
> > > undergoes default
> > > argument promotion to 'va_start' has undefined behavior [-Wvarargs]
> > > va_start(argp, h3);
> > > ^
> > > security/keys/trusted.c:126:37: note: parameter of type 'unsigned
> > > char' is declared here
> > > unsigned char *h2, unsigned char h3, ...)
> > > ^
> > > Specifically, it seems that both the C90 (4.8.1.1) and C11 (7.16.1.4)
> > > standards explicitly call this out as undefined behavior:
> > >
> > > The parameter parmN is the identifier of the rightmost parameter in
> > > the variable parameter list in the function definition (the one just
> > > before the ...). If the parameter parmN is declared with ... or with a
> > > type that is not compatible with the type that results after
> > > application of the default argument promotions, the behavior is
> > > undefined.
> > >
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/41
> > > Link: https://www.eskimo.com/~scs/cclass/int/sx11c.html
> > > Suggested-by: David Laight <David.Laight@aculab.com>
> > > Suggested-by: Denis Kenzior <denkenz@gmail.com>
> > > Suggested-by: James Bottomley <jejb@linux.vnet.ibm.com>
> > > Suggested-by: Nathan Chancellor <natechancellor@gmail.com>
> > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> >
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> >
> > /Jarkko
>
> Bumping the maintainers if this isn't already picked up?
James, Jarkko, or Mimi, can you please pick this up (and let me know
what tree it lands in)?
https://lkml.org/lkml/2018/10/23/116
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2] [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2019-02-11 18:36 ` Nick Desaulniers
0 siblings, 0 replies; 68+ messages in thread
From: Nick Desaulniers @ 2019-02-11 18:36 UTC (permalink / raw)
To: James E.J. Bottomley, jarkko.sakkinen, zohar
Cc: David Howells, Nathan Chancellor, David.Laight, denkenz, jmorris,
serge, keyrings, LKML, linux-integrity, linux-security-module
On Mon, Oct 29, 2018 at 10:54 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Wed, Oct 24, 2018 at 1:37 AM Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> >
> > On Mon, 22 Oct 2018, ndesaulniers@google.com wrote:
> > > Fixes the warning reported by Clang:
> > > security/keys/trusted.c:146:17: warning: passing an object that
> > > undergoes default
> > > argument promotion to 'va_start' has undefined behavior [-Wvarargs]
> > > va_start(argp, h3);
> > > ^
> > > security/keys/trusted.c:126:37: note: parameter of type 'unsigned
> > > char' is declared here
> > > unsigned char *h2, unsigned char h3, ...)
> > > ^
> > > Specifically, it seems that both the C90 (4.8.1.1) and C11 (7.16.1.4)
> > > standards explicitly call this out as undefined behavior:
> > >
> > > The parameter parmN is the identifier of the rightmost parameter in
> > > the variable parameter list in the function definition (the one just
> > > before the ...). If the parameter parmN is declared with ... or with a
> > > type that is not compatible with the type that results after
> > > application of the default argument promotions, the behavior is
> > > undefined.
> > >
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/41
> > > Link: https://www.eskimo.com/~scs/cclass/int/sx11c.html
> > > Suggested-by: David Laight <David.Laight@aculab.com>
> > > Suggested-by: Denis Kenzior <denkenz@gmail.com>
> > > Suggested-by: James Bottomley <jejb@linux.vnet.ibm.com>
> > > Suggested-by: Nathan Chancellor <natechancellor@gmail.com>
> > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> >
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> >
> > /Jarkko
>
> Bumping the maintainers if this isn't already picked up?
James, Jarkko, or Mimi, can you please pick this up (and let me know
what tree it lands in)?
https://lkml.org/lkml/2018/10/23/116
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2] [PATCH] KEYS: trusted: fix -Wvarags warning
2019-02-11 18:36 ` Nick Desaulniers
@ 2019-02-12 23:12 ` Jarkko Sakkinen
-1 siblings, 0 replies; 68+ messages in thread
From: Jarkko Sakkinen @ 2019-02-12 23:12 UTC (permalink / raw)
To: Nick Desaulniers
Cc: James E.J. Bottomley, zohar, David Howells, Nathan Chancellor,
David.Laight, denkenz, jmorris, serge, keyrings, LKML,
linux-integrity, linux-security-module
On Mon, Feb 11, 2019 at 10:36:51AM -0800, Nick Desaulniers wrote:
> On Mon, Oct 29, 2018 at 10:54 AM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > On Wed, Oct 24, 2018 at 1:37 AM Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com> wrote:
> > >
> > > On Mon, 22 Oct 2018, ndesaulniers@google.com wrote:
> > > > Fixes the warning reported by Clang:
> > > > security/keys/trusted.c:146:17: warning: passing an object that
> > > > undergoes default
> > > > argument promotion to 'va_start' has undefined behavior [-Wvarargs]
> > > > va_start(argp, h3);
> > > > ^
> > > > security/keys/trusted.c:126:37: note: parameter of type 'unsigned
> > > > char' is declared here
> > > > unsigned char *h2, unsigned char h3, ...)
> > > > ^
> > > > Specifically, it seems that both the C90 (4.8.1.1) and C11 (7.16.1.4)
> > > > standards explicitly call this out as undefined behavior:
> > > >
> > > > The parameter parmN is the identifier of the rightmost parameter in
> > > > the variable parameter list in the function definition (the one just
> > > > before the ...). If the parameter parmN is declared with ... or with a
> > > > type that is not compatible with the type that results after
> > > > application of the default argument promotions, the behavior is
> > > > undefined.
> > > >
> > > > Link: https://github.com/ClangBuiltLinux/linux/issues/41
> > > > Link: https://www.eskimo.com/~scs/cclass/int/sx11c.html
> > > > Suggested-by: David Laight <David.Laight@aculab.com>
> > > > Suggested-by: Denis Kenzior <denkenz@gmail.com>
> > > > Suggested-by: James Bottomley <jejb@linux.vnet.ibm.com>
> > > > Suggested-by: Nathan Chancellor <natechancellor@gmail.com>
> > > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > >
> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > >
> > > /Jarkko
> >
> > Bumping the maintainers if this isn't already picked up?
>
> James, Jarkko, or Mimi, can you please pick this up (and let me know
> what tree it lands in)?
I can volunteer. Have not done yet v5.1 PR so it would land to that
release. Is this agreed?
/Jarkko
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2] [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2019-02-12 23:12 ` Jarkko Sakkinen
0 siblings, 0 replies; 68+ messages in thread
From: Jarkko Sakkinen @ 2019-02-12 23:12 UTC (permalink / raw)
To: Nick Desaulniers
Cc: James E.J. Bottomley, zohar, David Howells, Nathan Chancellor,
David.Laight, denkenz, jmorris, serge, keyrings, LKML,
linux-integrity, linux-security-module
On Mon, Feb 11, 2019 at 10:36:51AM -0800, Nick Desaulniers wrote:
> On Mon, Oct 29, 2018 at 10:54 AM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > On Wed, Oct 24, 2018 at 1:37 AM Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com> wrote:
> > >
> > > On Mon, 22 Oct 2018, ndesaulniers@google.com wrote:
> > > > Fixes the warning reported by Clang:
> > > > security/keys/trusted.c:146:17: warning: passing an object that
> > > > undergoes default
> > > > argument promotion to 'va_start' has undefined behavior [-Wvarargs]
> > > > va_start(argp, h3);
> > > > ^
> > > > security/keys/trusted.c:126:37: note: parameter of type 'unsigned
> > > > char' is declared here
> > > > unsigned char *h2, unsigned char h3, ...)
> > > > ^
> > > > Specifically, it seems that both the C90 (4.8.1.1) and C11 (7.16.1.4)
> > > > standards explicitly call this out as undefined behavior:
> > > >
> > > > The parameter parmN is the identifier of the rightmost parameter in
> > > > the variable parameter list in the function definition (the one just
> > > > before the ...). If the parameter parmN is declared with ... or with a
> > > > type that is not compatible with the type that results after
> > > > application of the default argument promotions, the behavior is
> > > > undefined.
> > > >
> > > > Link: https://github.com/ClangBuiltLinux/linux/issues/41
> > > > Link: https://www.eskimo.com/~scs/cclass/int/sx11c.html
> > > > Suggested-by: David Laight <David.Laight@aculab.com>
> > > > Suggested-by: Denis Kenzior <denkenz@gmail.com>
> > > > Suggested-by: James Bottomley <jejb@linux.vnet.ibm.com>
> > > > Suggested-by: Nathan Chancellor <natechancellor@gmail.com>
> > > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > >
> > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > >
> > > /Jarkko
> >
> > Bumping the maintainers if this isn't already picked up?
>
> James, Jarkko, or Mimi, can you please pick this up (and let me know
> what tree it lands in)?
I can volunteer. Have not done yet v5.1 PR so it would land to that
release. Is this agreed?
/Jarkko
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2] [PATCH] KEYS: trusted: fix -Wvarags warning
2019-02-12 23:12 ` Jarkko Sakkinen
@ 2019-02-14 10:52 ` Jarkko Sakkinen
-1 siblings, 0 replies; 68+ messages in thread
From: Jarkko Sakkinen @ 2019-02-14 10:52 UTC (permalink / raw)
To: Nick Desaulniers
Cc: James E.J. Bottomley, zohar, David Howells, Nathan Chancellor,
David.Laight, denkenz, jmorris, serge, keyrings, LKML,
linux-integrity, linux-security-module
On Wed, Feb 13, 2019 at 01:12:56AM +0200, Jarkko Sakkinen wrote:
> On Mon, Feb 11, 2019 at 10:36:51AM -0800, Nick Desaulniers wrote:
> > On Mon, Oct 29, 2018 at 10:54 AM Nick Desaulniers
> > <ndesaulniers@google.com> wrote:
> > >
> > > On Wed, Oct 24, 2018 at 1:37 AM Jarkko Sakkinen
> > > <jarkko.sakkinen@linux.intel.com> wrote:
> > > >
> > > > On Mon, 22 Oct 2018, ndesaulniers@google.com wrote:
> > > > > Fixes the warning reported by Clang:
> > > > > security/keys/trusted.c:146:17: warning: passing an object that
> > > > > undergoes default
> > > > > argument promotion to 'va_start' has undefined behavior [-Wvarargs]
> > > > > va_start(argp, h3);
> > > > > ^
> > > > > security/keys/trusted.c:126:37: note: parameter of type 'unsigned
> > > > > char' is declared here
> > > > > unsigned char *h2, unsigned char h3, ...)
> > > > > ^
> > > > > Specifically, it seems that both the C90 (4.8.1.1) and C11 (7.16.1.4)
> > > > > standards explicitly call this out as undefined behavior:
> > > > >
> > > > > The parameter parmN is the identifier of the rightmost parameter in
> > > > > the variable parameter list in the function definition (the one just
> > > > > before the ...). If the parameter parmN is declared with ... or with a
> > > > > type that is not compatible with the type that results after
> > > > > application of the default argument promotions, the behavior is
> > > > > undefined.
> > > > >
> > > > > Link: https://github.com/ClangBuiltLinux/linux/issues/41
> > > > > Link: https://www.eskimo.com/~scs/cclass/int/sx11c.html
> > > > > Suggested-by: David Laight <David.Laight@aculab.com>
> > > > > Suggested-by: Denis Kenzior <denkenz@gmail.com>
> > > > > Suggested-by: James Bottomley <jejb@linux.vnet.ibm.com>
> > > > > Suggested-by: Nathan Chancellor <natechancellor@gmail.com>
> > > > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > > >
> > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > >
> > > > /Jarkko
> > >
> > > Bumping the maintainers if this isn't already picked up?
> >
> > James, Jarkko, or Mimi, can you please pick this up (and let me know
> > what tree it lands in)?
>
> I can volunteer. Have not done yet v5.1 PR so it would land to that
> release. Is this agreed?
http://git.infradead.org/users/jjs/linux-tpmdd.git/commit/cfb1f7ee3b35e6ba9e9e2de53a8668ced6397f88
/Jarkko
^ permalink raw reply [flat|nested] 68+ messages in thread
* Re: [PATCH v2] [PATCH] KEYS: trusted: fix -Wvarags warning
@ 2019-02-14 10:52 ` Jarkko Sakkinen
0 siblings, 0 replies; 68+ messages in thread
From: Jarkko Sakkinen @ 2019-02-14 10:52 UTC (permalink / raw)
To: Nick Desaulniers
Cc: James E.J. Bottomley, zohar, David Howells, Nathan Chancellor,
David.Laight, denkenz, jmorris, serge, keyrings, LKML,
linux-integrity, linux-security-module
On Wed, Feb 13, 2019 at 01:12:56AM +0200, Jarkko Sakkinen wrote:
> On Mon, Feb 11, 2019 at 10:36:51AM -0800, Nick Desaulniers wrote:
> > On Mon, Oct 29, 2018 at 10:54 AM Nick Desaulniers
> > <ndesaulniers@google.com> wrote:
> > >
> > > On Wed, Oct 24, 2018 at 1:37 AM Jarkko Sakkinen
> > > <jarkko.sakkinen@linux.intel.com> wrote:
> > > >
> > > > On Mon, 22 Oct 2018, ndesaulniers@google.com wrote:
> > > > > Fixes the warning reported by Clang:
> > > > > security/keys/trusted.c:146:17: warning: passing an object that
> > > > > undergoes default
> > > > > argument promotion to 'va_start' has undefined behavior [-Wvarargs]
> > > > > va_start(argp, h3);
> > > > > ^
> > > > > security/keys/trusted.c:126:37: note: parameter of type 'unsigned
> > > > > char' is declared here
> > > > > unsigned char *h2, unsigned char h3, ...)
> > > > > ^
> > > > > Specifically, it seems that both the C90 (4.8.1.1) and C11 (7.16.1.4)
> > > > > standards explicitly call this out as undefined behavior:
> > > > >
> > > > > The parameter parmN is the identifier of the rightmost parameter in
> > > > > the variable parameter list in the function definition (the one just
> > > > > before the ...). If the parameter parmN is declared with ... or with a
> > > > > type that is not compatible with the type that results after
> > > > > application of the default argument promotions, the behavior is
> > > > > undefined.
> > > > >
> > > > > Link: https://github.com/ClangBuiltLinux/linux/issues/41
> > > > > Link: https://www.eskimo.com/~scs/cclass/int/sx11c.html
> > > > > Suggested-by: David Laight <David.Laight@aculab.com>
> > > > > Suggested-by: Denis Kenzior <denkenz@gmail.com>
> > > > > Suggested-by: James Bottomley <jejb@linux.vnet.ibm.com>
> > > > > Suggested-by: Nathan Chancellor <natechancellor@gmail.com>
> > > > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > > >
> > > > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > >
> > > > /Jarkko
> > >
> > > Bumping the maintainers if this isn't already picked up?
> >
> > James, Jarkko, or Mimi, can you please pick this up (and let me know
> > what tree it lands in)?
>
> I can volunteer. Have not done yet v5.1 PR so it would land to that
> release. Is this agreed?
http://git.infradead.org/users/jjs/linux-tpmdd.git/commit/cfb1f7ee3b35e6ba9e9e2de53a8668ced6397f88
/Jarkko
^ permalink raw reply [flat|nested] 68+ messages in thread