| Message ID | 20181011203126.15338-1-ndesaulniers@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KEYS: trusted: fix -Wvarags warning | expand |
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 >
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
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
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
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
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
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
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
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
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 >
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 > >
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
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
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 >
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 > > >
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
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
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
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.
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)
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)
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)
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;
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(-)