Bug #13968
closedtpm driver fails bounds checks
100%
Description
Noticing that usr/src/uts/intel/tpm/Makefile
currently silencing array bounds errors (as part of the initial smatch implementation), removing the silence gives the following errors:
/ws/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: /ws/illumos-gate/usr/src/uts/common/io/tpm/tpm.c:691 tpm_get_ordinal_duration() error: buffer overflow 'tsc_ords_duration' 12 <= 12 /ws/illumos-gate/usr/src/tools/proto/root_i386-nd/opt/onbld/bin/i386/smatch: /ws/illumos-gate/usr/src/uts/common/io/tpm/tpm.c:701 tpm_get_ordinal_duration() error: buffer overflow 'tpm_ords_duration' 243 <= 243
Looking at the relevant code, we see:
if (ordinal & TSC_ORDINAL_MASK) {
if (ordinal > TSC_ORDINAL_MAX) {
#ifdef DEBUG
cmn_err(CE_WARN,
"!%s: tsc ordinal: %d exceeds MAX: %d",
myname, ordinal, TSC_ORDINAL_MAX);
#endif
return (0);
}
index = tsc_ords_duration[ordinal];
} else {
if (ordinal > TPM_ORDINAL_MAX) {
#ifdef DEBUG
cmn_err(CE_WARN,
"!%s: ordinal %d exceeds MAX: %d",
myname, ordinal, TPM_ORDINAL_MAX);
#endif
return (0);
}
index = tpm_ords_duration[ordinal];
}
Looking at usr/src/uts/common/io/tpm_tis.h
, we see:
#define TSC_ORDINAL_MAX 12
#define TPM_ORDINAL_MAX 243
and in usr/src/uts/common/io/tpm_duration.h
:
static const TPM_DURATION_T tpm_ords_duration[TPM_ORDINAL_MAX] = {
...
};
...
static const uint8_t tsc_ords_duration[TSC_ORDINAL_MAX] = {
...
};
Smatch is correctly identifying a problem -- the if checks should be using >=
, not >
to validate the ordinal value is in bounds.
Since the tpm device has 0600 root/sys perms (i.e. it requires root access to open and use), this doesn't appear to be a security issue, but we should fix this given how trivial it is.
Related issues
Updated by Jason King over 2 years ago
To test this (and 13969), AndyF was had a TPM1.2 module available and was able to use pktool to initialize the TPM, set the SO pin and generate a key.
Updated by Dan McDonald over 2 years ago
- Related to Bug #13969: tpm driver should use __func__ added
Updated by Electric Monk over 2 years ago
- Status changed from New to Closed
- % Done changed from 0 to 100
git commit e9fe7b359091f8e565041c286948edcb4e1e96f6
commit e9fe7b359091f8e565041c286948edcb4e1e96f6 Author: Jason King <jason.brian.king@gmail.com> Date: 2021-08-11T15:31:53.000Z 13968 tpm driver fails bounds checks 13969 tpm driver should use __func__ Reviewed by: Toomas Soome <tsoome@me.com> Reviewed by: Peter Tribble <peter.tribble@gmail.com> Approved by: Dan McDonald <danmcd@joyent.com>