Project

General

Profile

Actions

Bug #13968

closed

tpm driver fails bounds checks

Added by Jason King over 2 years ago. Updated over 2 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
driver - device drivers
Start date:
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
Gerrit CR:
External Bug:

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

Related to illumos gate - Bug #13969: tpm driver should use __func__ClosedJason King

Actions
Actions #1

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.

Actions #2

Updated by Dan McDonald over 2 years ago

  • Related to Bug #13969: tpm driver should use __func__ added
Actions #3

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>

Actions

Also available in: Atom PDF