[v2] tpm-dev-common: Reject too short writes
diff mbox

Message ID 20170904173642.5988-1-Alexander.Steffen@infineon.com
State New
Headers show

Commit Message

Alexander Steffen Sept. 4, 2017, 5:36 p.m. UTC
tpm_transmit() does not offer an explicit interface to indicate the number
of valid bytes in the communication buffer. Instead, it relies on the
commandSize field in the TPM header that is encoded within the buffer.
Therefore, ensure that a) enough data has been written to the buffer, so
that the commandSize field is present and b) the commandSize field does not
announce more data than has been written to the buffer.

This should have been fixed with CVE-2011-1161 long ago, but apparently
a correct version of that patch never made it into the kernel.

Cc: stable@vger.kernel.org
Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
---
v2:
- Moved all changes to tpm_common_write in a single patch.

 drivers/char/tpm/tpm-dev-common.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Jarkko Sakkinen Sept. 5, 2017, 7:05 p.m. UTC | #1
On Mon, Sep 04, 2017 at 07:36:42PM +0200, Alexander Steffen wrote:
> tpm_transmit() does not offer an explicit interface to indicate the number
> of valid bytes in the communication buffer. Instead, it relies on the
> commandSize field in the TPM header that is encoded within the buffer.
> Therefore, ensure that a) enough data has been written to the buffer, so
> that the commandSize field is present and b) the commandSize field does not
> announce more data than has been written to the buffer.
> 
> This should have been fixed with CVE-2011-1161 long ago, but apparently
> a correct version of that patch never made it into the kernel.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> ---
> v2:
> - Moved all changes to tpm_common_write in a single patch.
> 
>  drivers/char/tpm/tpm-dev-common.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
> index 610638a..ac25574 100644
> --- a/drivers/char/tpm/tpm-dev-common.c
> +++ b/drivers/char/tpm/tpm-dev-common.c
> @@ -99,7 +99,8 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
>  	if (atomic_read(&priv->data_pending) != 0)
>  		return -EBUSY;
>  
> -	if (in_size > TPM_BUFSIZE)
> +	if (in_size > sizeof(priv->data_buffer) || in_size < 6 ||
> +	    in_size < be32_to_cpu(*((__be32 *) (buf + 2))))
>  		return -E2BIG;
>  
>  	mutex_lock(&priv->buffer_mutex);
> -- 
> 2.7.4
> 

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

There's now some delay getting patches to my git tree because next week
is conference week and I have lots of stuff to do before I depart
Finland. I'm sorry about that.

At latest I push these during the plane trip (I can remotely access
test machines with plane internet connection, not the first time I'm
doing this).

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarkko Sakkinen Sept. 6, 2017, 12:42 p.m. UTC | #2
On Mon, Sep 04, 2017 at 07:36:42PM +0200, Alexander Steffen wrote:
> tpm_transmit() does not offer an explicit interface to indicate the number
> of valid bytes in the communication buffer. Instead, it relies on the
> commandSize field in the TPM header that is encoded within the buffer.
> Therefore, ensure that a) enough data has been written to the buffer, so
> that the commandSize field is present and b) the commandSize field does not
> announce more data than has been written to the buffer.
> 
> This should have been fixed with CVE-2011-1161 long ago, but apparently
> a correct version of that patch never made it into the kernel.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> ---
> v2:
> - Moved all changes to tpm_common_write in a single patch.
> 
>  drivers/char/tpm/tpm-dev-common.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
> index 610638a..ac25574 100644
> --- a/drivers/char/tpm/tpm-dev-common.c
> +++ b/drivers/char/tpm/tpm-dev-common.c
> @@ -99,7 +99,8 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
>  	if (atomic_read(&priv->data_pending) != 0)
>  		return -EBUSY;
>  
> -	if (in_size > TPM_BUFSIZE)
> +	if (in_size > sizeof(priv->data_buffer) || in_size < 6 ||
> +	    in_size < be32_to_cpu(*((__be32 *) (buf + 2))))
>  		return -E2BIG;
>  
>  	mutex_lock(&priv->buffer_mutex);
> -- 
> 2.7.4
> 

How did you test this change after you implemented it?

Just thinking what to add to https://github.com/jsakkine-intel/tpm2-scripts

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarkko Sakkinen Sept. 6, 2017, 12:50 p.m. UTC | #3
On Wed, Sep 06, 2017 at 03:42:33PM +0300, Jarkko Sakkinen wrote:
> On Mon, Sep 04, 2017 at 07:36:42PM +0200, Alexander Steffen wrote:
> > tpm_transmit() does not offer an explicit interface to indicate the number
> > of valid bytes in the communication buffer. Instead, it relies on the
> > commandSize field in the TPM header that is encoded within the buffer.
> > Therefore, ensure that a) enough data has been written to the buffer, so
> > that the commandSize field is present and b) the commandSize field does not
> > announce more data than has been written to the buffer.
> > 
> > This should have been fixed with CVE-2011-1161 long ago, but apparently
> > a correct version of that patch never made it into the kernel.
> > 
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> > ---
> > v2:
> > - Moved all changes to tpm_common_write in a single patch.
> > 
> >  drivers/char/tpm/tpm-dev-common.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
> > index 610638a..ac25574 100644
> > --- a/drivers/char/tpm/tpm-dev-common.c
> > +++ b/drivers/char/tpm/tpm-dev-common.c
> > @@ -99,7 +99,8 @@ ssize_t tpm_common_write(struct file *file, const char __user *buf,
> >  	if (atomic_read(&priv->data_pending) != 0)
> >  		return -EBUSY;
> >  
> > -	if (in_size > TPM_BUFSIZE)
> > +	if (in_size > sizeof(priv->data_buffer) || in_size < 6 ||
> > +	    in_size < be32_to_cpu(*((__be32 *) (buf + 2))))
> >  		return -E2BIG;
> >  
> >  	mutex_lock(&priv->buffer_mutex);
> > -- 
> > 2.7.4
> > 
> 
> How did you test this change after you implemented it?
> 
> Just thinking what to add to https://github.com/jsakkine-intel/tpm2-scripts
> 
> /Jarkko

Just when I started to implement this that the bug fix itself does not
have yet the right semantics.

It should be just add a new check:

if (in_size != be32_to_cpu(*((__be32 *) (buf + 2))))
	return -EINVAL;

The existing check is correct. This was missing. The reason for this is
that we process whatever is in the in_size bytes as a full command.

Sorry I didn't notice before I started to implement a test case.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Steffen Sept. 6, 2017, 2:19 p.m. UTC | #4
PiBPbiBXZWQsIFNlcCAwNiwgMjAxNyBhdCAwMzo0MjozM1BNICswMzAwLCBKYXJra28gU2Fra2lu
ZW4gd3JvdGU6DQo+ID4gT24gTW9uLCBTZXAgMDQsIDIwMTcgYXQgMDc6MzY6NDJQTSArMDIwMCwg
QWxleGFuZGVyIFN0ZWZmZW4gd3JvdGU6DQo+ID4gPiB0cG1fdHJhbnNtaXQoKSBkb2VzIG5vdCBv
ZmZlciBhbiBleHBsaWNpdCBpbnRlcmZhY2UgdG8gaW5kaWNhdGUgdGhlDQo+ID4gPiBudW1iZXIg
b2YgdmFsaWQgYnl0ZXMgaW4gdGhlIGNvbW11bmljYXRpb24gYnVmZmVyLiBJbnN0ZWFkLCBpdA0K
PiA+ID4gcmVsaWVzIG9uIHRoZSBjb21tYW5kU2l6ZSBmaWVsZCBpbiB0aGUgVFBNIGhlYWRlciB0
aGF0IGlzIGVuY29kZWQgd2l0aGluDQo+IHRoZSBidWZmZXIuDQo+ID4gPiBUaGVyZWZvcmUsIGVu
c3VyZSB0aGF0IGEpIGVub3VnaCBkYXRhIGhhcyBiZWVuIHdyaXR0ZW4gdG8gdGhlDQo+ID4gPiBi
dWZmZXIsIHNvIHRoYXQgdGhlIGNvbW1hbmRTaXplIGZpZWxkIGlzIHByZXNlbnQgYW5kIGIpIHRo
ZQ0KPiA+ID4gY29tbWFuZFNpemUgZmllbGQgZG9lcyBub3QgYW5ub3VuY2UgbW9yZSBkYXRhIHRo
YW4gaGFzIGJlZW4gd3JpdHRlbg0KPiB0byB0aGUgYnVmZmVyLg0KPiA+ID4NCj4gPiA+IFRoaXMg
c2hvdWxkIGhhdmUgYmVlbiBmaXhlZCB3aXRoIENWRS0yMDExLTExNjEgbG9uZyBhZ28sIGJ1dA0K
PiA+ID4gYXBwYXJlbnRseSBhIGNvcnJlY3QgdmVyc2lvbiBvZiB0aGF0IHBhdGNoIG5ldmVyIG1h
ZGUgaXQgaW50byB0aGUga2VybmVsLg0KPiA+ID4NCj4gPiA+IENjOiBzdGFibGVAdmdlci5rZXJu
ZWwub3JnDQo+ID4gPiBTaWduZWQtb2ZmLWJ5OiBBbGV4YW5kZXIgU3RlZmZlbiA8QWxleGFuZGVy
LlN0ZWZmZW5AaW5maW5lb24uY29tPg0KPiA+ID4gLS0tDQo+ID4gPiB2MjoNCj4gPiA+IC0gTW92
ZWQgYWxsIGNoYW5nZXMgdG8gdHBtX2NvbW1vbl93cml0ZSBpbiBhIHNpbmdsZSBwYXRjaC4NCj4g
PiA+DQo+ID4gPiAgZHJpdmVycy9jaGFyL3RwbS90cG0tZGV2LWNvbW1vbi5jIHwgMyArKy0NCj4g
PiA+ICAxIGZpbGUgY2hhbmdlZCwgMiBpbnNlcnRpb25zKCspLCAxIGRlbGV0aW9uKC0pDQo+ID4g
Pg0KPiA+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvY2hhci90cG0vdHBtLWRldi1jb21tb24uYw0K
PiA+ID4gYi9kcml2ZXJzL2NoYXIvdHBtL3RwbS1kZXYtY29tbW9uLmMNCj4gPiA+IGluZGV4IDYx
MDYzOGEuLmFjMjU1NzQgMTAwNjQ0DQo+ID4gPiAtLS0gYS9kcml2ZXJzL2NoYXIvdHBtL3RwbS1k
ZXYtY29tbW9uLmMNCj4gPiA+ICsrKyBiL2RyaXZlcnMvY2hhci90cG0vdHBtLWRldi1jb21tb24u
Yw0KPiA+ID4gQEAgLTk5LDcgKzk5LDggQEAgc3NpemVfdCB0cG1fY29tbW9uX3dyaXRlKHN0cnVj
dCBmaWxlICpmaWxlLCBjb25zdA0KPiBjaGFyIF9fdXNlciAqYnVmLA0KPiA+ID4gIAlpZiAoYXRv
bWljX3JlYWQoJnByaXYtPmRhdGFfcGVuZGluZykgIT0gMCkNCj4gPiA+ICAJCXJldHVybiAtRUJV
U1k7DQo+ID4gPg0KPiA+ID4gLQlpZiAoaW5fc2l6ZSA+IFRQTV9CVUZTSVpFKQ0KPiA+ID4gKwlp
ZiAoaW5fc2l6ZSA+IHNpemVvZihwcml2LT5kYXRhX2J1ZmZlcikgfHwgaW5fc2l6ZSA8IDYgfHwN
Cj4gPiA+ICsJICAgIGluX3NpemUgPCBiZTMyX3RvX2NwdSgqKChfX2JlMzIgKikgKGJ1ZiArIDIp
KSkpDQo+ID4gPiAgCQlyZXR1cm4gLUUyQklHOw0KPiA+ID4NCj4gPiA+ICAJbXV0ZXhfbG9jaygm
cHJpdi0+YnVmZmVyX211dGV4KTsNCj4gPiA+IC0tDQo+ID4gPiAyLjcuNA0KPiA+ID4NCj4gPg0K
PiA+IEhvdyBkaWQgeW91IHRlc3QgdGhpcyBjaGFuZ2UgYWZ0ZXIgeW91IGltcGxlbWVudGVkIGl0
Pw0KPiA+DQo+ID4gSnVzdCB0aGlua2luZyB3aGF0IHRvIGFkZCB0bw0KPiA+IGh0dHBzOi8vZ2l0
aHViLmNvbS9qc2Fra2luZS1pbnRlbC90cG0yLXNjcmlwdHMNCg0KSSBhbHJlYWR5IGhhZCB0ZXN0
IGNhc2VzIHRoYXQgZmFpbGVkIHdpdGggc29tZSBvZiBteSBUUE1zIHVuZGVyIHNvbWUgY2lyY3Vt
c3RhbmNlcy4gSSdsbCB0cnkgdG8gY29tZSB1cCB3aXRoIGEgY29uY2lzZSBkZXNjcmlwdGlvbiBv
ZiB3aGF0IHRob3NlIHRlc3RzIGRvIG9yIHNlbmQgeW91IGEgcGF0Y2ggZGlyZWN0bHkgZm9yIHlv
dXIgdGVzdHMuIEdpdEh1YiBwdWxsIHJlcXVlc3RzIGFyZSBva2F5IGZvciB0aGF0IHJlcG9zaXRv
cnk/IChJIGFscmVhZHkgaGF2ZSBvbmUgd2FpdGluZyB0aGVyZS4pDQoNCj4gPg0KPiA+IC9KYXJr
a28NCj4gDQo+IEp1c3Qgd2hlbiBJIHN0YXJ0ZWQgdG8gaW1wbGVtZW50IHRoaXMgdGhhdCB0aGUg
YnVnIGZpeCBpdHNlbGYgZG9lcyBub3QgaGF2ZSB5ZXQNCj4gdGhlIHJpZ2h0IHNlbWFudGljcy4N
Cj4gDQo+IEl0IHNob3VsZCBiZSBqdXN0IGFkZCBhIG5ldyBjaGVjazoNCj4gDQo+IGlmIChpbl9z
aXplICE9IGJlMzJfdG9fY3B1KCooKF9fYmUzMiAqKSAoYnVmICsgMikpKSkNCj4gCXJldHVybiAt
RUlOVkFMOw0KPiANCj4gVGhlIGV4aXN0aW5nIGNoZWNrIGlzIGNvcnJlY3QuIFRoaXMgd2FzIG1p
c3NpbmcuIFRoZSByZWFzb24gZm9yIHRoaXMgaXMgdGhhdCB3ZQ0KPiBwcm9jZXNzIHdoYXRldmVy
IGlzIGluIHRoZSBpbl9zaXplIGJ5dGVzIGFzIGEgZnVsbCBjb21tYW5kLg0KDQpUaGVyZSBhcmUg
dHdvIHByb2JsZW1zIHdpdGggdGhpcyBzb2x1dGlvbjoNCg0KMS4gWW91IG5lZWQgdG8gY2hlY2sg
Zm9yIGluX3NpemUgPCA2LCBvdGhlcndpc2UgeW91IHJlYWQgZGF0YSB0aGF0IGhhcyBub3QgYmVl
biB3cml0dGVuIHRoZXJlIGR1cmluZyB0aGF0IHJlcXVlc3QuIEkgaGF2ZW4ndCB0ZXN0ZWQgdGhp
cywgYnV0IEknZCBleHBlY3QgaXQgdG8gZmFpbCBmb3IgZXhhbXBsZSBpZiB5b3UgdHJ5IHRvIHNl
bmQgdGhlIHR3byBjb21tYW5kcyAiMDAgMDAgMDAgMDAgMDAgMDIiIGFuZCAiMDAgMDAiLiBUaGUg
Zmlyc3Qgd2lsbCBiZSByZWplY3RlZCB3aXRoIEVJTlZBTCwgYmVjYXVzZSA2IChpbl9zaXplKSAh
PSAyIChjb21tYW5kU2l6ZSkuIEJ1dCB0aGUgc2Vjb25kIHdpbGwgcGFzcyB0aGF0IGNoZWNrLCBi
ZWNhdXNlIG5vdyBpbl9zaXplIGhhcHBlbnMgdG8gbWF0Y2ggdGhlIGNvbW1hbmRTaXplIHRoYXQg
aGFzIG9ubHkgYmVlbiB3cml0dGVuIHRvIHRoZSBidWZmZXIgZm9yIHRoZSBmaXJzdCBjb21tYW5k
Lg0KDQoyLiBZb3UgbWF5IG5vdCByZWplY3QgY29tbWFuZHMgd2hlcmUgaW5fc2l6ZSA+IGNvbW1h
bmRTaXplLCBiZWNhdXNlIFRJUy9QVFAgcmVxdWlyZSB0aGUgVFBNIHRvIHRocm93IGF3YXkgZXh0
cmEgYnl0ZXMgKGFuZCBwcm9jZXNzIHRoZSBjb21tYW5kIGFzIHVzdWFsKSwgbm90IGZhaWwgdGhl
IGNvbW1hbmQuIFlvdSBjYW4gc2VlIHRoYXQgaW4gdGhlIFN0YXRlIFRyYW5zaXRpb24gVGFibGUg
KFRhYmxlIDE4IGluIFRJUyAxLjMpLCBsaW5lIDIwLCB3aXRoIHRoZSBUUE0gaW4gUmVjZXB0aW9u
IHN0YXRlIGFuZCBFeHBlY3Q9MCwgd3JpdGluZyBtb3JlIGRhdGEgZG9lcyBub3QgY2hhbmdlIHRo
ZSBzdGF0ZSAoIldyaXRlIGlzIG5vdCBleHBlY3RlZC4gRHJvcCB3cml0ZS4gVFBNIGlnbm9yZXMg
dGhpcyBzdGF0ZSB0cmFuc2l0aW9uLiIpLiBPZiBjb3Vyc2UsIHNpbmNlIHdlIGRvIG5vdCBwYXNz
IG9uIGluX3NpemUsIGJ1dCBvbmx5IGNvbW1hbmRTaXplIHRoZSBUUE0gd2lsbCBuZXZlciBzZWUg
dGhvc2UgZXh0cmEgYnl0ZXMsIGJ1dCB0aGUgZXh0ZXJuYWwgYmVoYXZpb3IgKGZvciB1c2VyIHNw
YWNlIGFwcGxpY2F0aW9ucykgaXMgaWRlbnRpY2FsLg0KDQpXaGVuIHlvdSBmaXggdGhvc2UgdHdv
IHByb2JsZW1zLCB5b3UncmUgcHJvYmFibHkgYmFjayBhdCBteSBzb2x1dGlvbi4gVGhlIG9ubHkg
b3RoZXIgY2hhbmdlIEkgbWFkZSAocmVuYW1pbmcgVFBNX0JVRlNJWkUgdG8gc2l6ZW9mKHByaXYt
PmRhdGFfYnVmZmVyKSkgZG9lcyBub3QgY2hhbmdlIGFueXRoaW5nLCB0aGUgdmFsdWVzIGFyZSBp
ZGVudGljYWwuIEkganVzdCBmaW5kIGl0IHZlcnkgY29uZnVzaW5nIHdoZW4geW91IGNvbXBhcmUg
c29tZXRoaW5nIGFnYWluc3QgYSBtYWdpYyBjb25zdGFudCB0byBhdm9pZCBidWZmZXIgb3ZlcmZs
b3dzIGluIHlvdXIgbWVtY3B5LiBVc2luZyB0aGUgYnVmZmVyIHNpemUgZGlyZWN0bHkgaXMgbXVj
aCBtb3JlIHN0cmFpZ2h0Zm9yd2FyZCAoYW5kIGxlc3MgcHJvbmUgdG8gZmFpbCBhcyBzb29uIGFz
IHNvbWVvbmUgY2hhbmdlcyB0aGUgc3RydWN0dXJlIGRlZmluaXRpb24pLg0KDQo+IA0KPiBTb3Jy
eSBJIGRpZG4ndCBub3RpY2UgYmVmb3JlIEkgc3RhcnRlZCB0byBpbXBsZW1lbnQgYSB0ZXN0IGNh
c2UuDQo+IA0KPiAvSmFya2tvDQoNCkFsZXhhbmRlcg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jarkko Sakkinen Sept. 7, 2017, 4:46 p.m. UTC | #5
On Wed, Sep 06, 2017 at 02:19:28PM +0000, Alexander.Steffen@infineon.com wrote:
> > On Wed, Sep 06, 2017 at 03:42:33PM +0300, Jarkko Sakkinen wrote:
> > > On Mon, Sep 04, 2017 at 07:36:42PM +0200, Alexander Steffen wrote:
> > > > tpm_transmit() does not offer an explicit interface to indicate the
> > > > number of valid bytes in the communication buffer. Instead, it
> > > > relies on the commandSize field in the TPM header that is encoded within
> > the buffer.
> > > > Therefore, ensure that a) enough data has been written to the
> > > > buffer, so that the commandSize field is present and b) the
> > > > commandSize field does not announce more data than has been written
> > to the buffer.
> > > >
> > > > This should have been fixed with CVE-2011-1161 long ago, but
> > > > apparently a correct version of that patch never made it into the kernel.
> > > >
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Alexander Steffen <Alexander.Steffen@infineon.com>
> > > > ---
> > > > v2:
> > > > - Moved all changes to tpm_common_write in a single patch.
> > > >
> > > >  drivers/char/tpm/tpm-dev-common.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/char/tpm/tpm-dev-common.c
> > > > b/drivers/char/tpm/tpm-dev-common.c
> > > > index 610638a..ac25574 100644
> > > > --- a/drivers/char/tpm/tpm-dev-common.c
> > > > +++ b/drivers/char/tpm/tpm-dev-common.c
> > > > @@ -99,7 +99,8 @@ ssize_t tpm_common_write(struct file *file, const
> > char __user *buf,
> > > >  	if (atomic_read(&priv->data_pending) != 0)
> > > >  		return -EBUSY;
> > > >
> > > > -	if (in_size > TPM_BUFSIZE)
> > > > +	if (in_size > sizeof(priv->data_buffer) || in_size < 6 ||
> > > > +	    in_size < be32_to_cpu(*((__be32 *) (buf + 2))))
> > > >  		return -E2BIG;
> > > >
> > > >  	mutex_lock(&priv->buffer_mutex);
> > > > --
> > > > 2.7.4
> > > >
> > >
> > > How did you test this change after you implemented it?
> > >
> > > Just thinking what to add to
> > > https://github.com/jsakkine-intel/tpm2-scripts
> 
> I already had test cases that failed with some of my TPMs under some circumstances. I'll try to come up with a concise description of what those tests do or send you a patch directly for your tests. GitHub pull requests are okay for that repository? (I already have one waiting there.)
> 
> > >
> > > /Jarkko
> > 
> > Just when I started to implement this that the bug fix itself does not have yet
> > the right semantics.
> > 
> > It should be just add a new check:
> > 
> > if (in_size != be32_to_cpu(*((__be32 *) (buf + 2))))
> > 	return -EINVAL;
> > 
> > The existing check is correct. This was missing. The reason for this is that we
> > process whatever is in the in_size bytes as a full command.
> 
> There are two problems with this solution:
> 
> 1. You need to check for in_size < 6, otherwise you read data that has
> not been written there during that request. I haven't tested this, but
> I'd expect it to fail for example if you try to send the two commands
> "00 00 00 00 00 02" and "00 00". The first will be rejected with
> EINVAL, because 6 (in_size) != 2 (commandSize). But the second will
> pass that check, because now in_size happens to match the commandSize
> that has only been written to the buffer for the first command.

AFAIK tpm_transmit checks that the command has at least the header.

> 2. You may not reject commands where in_size > commandSize, because
> TIS/PTP require the TPM to throw away extra bytes (and process the
> command as usual), not fail the command. You can see that in the State
> Transition Table (Table 18 in TIS 1.3), line 20, with the TPM in
> Reception state and Expect=0, writing more data does not change the
> state ("Write is not expected. Drop write. TPM ignores this state
> transition."). Of course, since we do not pass on in_size, but only
> commandSize the TPM will never see those extra bytes, but the external
> behavior (for user space applications) is identical.

OK, this is more relevant. What is the legit case to send extra bytes?

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Steffen Sept. 8, 2017, 2:26 p.m. UTC | #6
> On Wed, Sep 06, 2017 at 02:19:28PM +0000,

> Alexander.Steffen@infineon.com wrote:

> > > On Wed, Sep 06, 2017 at 03:42:33PM +0300, Jarkko Sakkinen wrote:

> > > > On Mon, Sep 04, 2017 at 07:36:42PM +0200, Alexander Steffen wrote:

> > > > > tpm_transmit() does not offer an explicit interface to indicate the

> > > > > number of valid bytes in the communication buffer. Instead, it

> > > > > relies on the commandSize field in the TPM header that is encoded

> within

> > > the buffer.

> > > > > Therefore, ensure that a) enough data has been written to the

> > > > > buffer, so that the commandSize field is present and b) the

> > > > > commandSize field does not announce more data than has been

> written

> > > to the buffer.

> > > > >

> > > > > This should have been fixed with CVE-2011-1161 long ago, but

> > > > > apparently a correct version of that patch never made it into the

> kernel.

> > > > >

> > > > > Cc: stable@vger.kernel.org

> > > > > Signed-off-by: Alexander Steffen

> <Alexander.Steffen@infineon.com>

> > > > > ---

> > > > > v2:

> > > > > - Moved all changes to tpm_common_write in a single patch.

> > > > >

> > > > >  drivers/char/tpm/tpm-dev-common.c | 3 ++-

> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)

> > > > >

> > > > > diff --git a/drivers/char/tpm/tpm-dev-common.c

> > > > > b/drivers/char/tpm/tpm-dev-common.c

> > > > > index 610638a..ac25574 100644

> > > > > --- a/drivers/char/tpm/tpm-dev-common.c

> > > > > +++ b/drivers/char/tpm/tpm-dev-common.c

> > > > > @@ -99,7 +99,8 @@ ssize_t tpm_common_write(struct file *file,

> const

> > > char __user *buf,

> > > > >  	if (atomic_read(&priv->data_pending) != 0)

> > > > >  		return -EBUSY;

> > > > >

> > > > > -	if (in_size > TPM_BUFSIZE)

> > > > > +	if (in_size > sizeof(priv->data_buffer) || in_size < 6 ||

> > > > > +	    in_size < be32_to_cpu(*((__be32 *) (buf + 2))))

> > > > >  		return -E2BIG;

> > > > >

> > > > >  	mutex_lock(&priv->buffer_mutex);

> > > > > --

> > > > > 2.7.4

> > > > >

> > > >

> > > > How did you test this change after you implemented it?

> > > >

> > > > Just thinking what to add to

> > > > https://github.com/jsakkine-intel/tpm2-scripts

> >

> > I already had test cases that failed with some of my TPMs under some

> circumstances. I'll try to come up with a concise description of what those

> tests do or send you a patch directly for your tests. GitHub pull requests are

> okay for that repository? (I already have one waiting there.)

> >

> > > >

> > > > /Jarkko

> > >

> > > Just when I started to implement this that the bug fix itself does not have

> yet

> > > the right semantics.

> > >

> > > It should be just add a new check:

> > >

> > > if (in_size != be32_to_cpu(*((__be32 *) (buf + 2))))

> > > 	return -EINVAL;

> > >

> > > The existing check is correct. This was missing. The reason for this is that

> we

> > > process whatever is in the in_size bytes as a full command.

> >

> > There are two problems with this solution:

> >

> > 1. You need to check for in_size < 6, otherwise you read data that has

> > not been written there during that request. I haven't tested this, but

> > I'd expect it to fail for example if you try to send the two commands

> > "00 00 00 00 00 02" and "00 00". The first will be rejected with

> > EINVAL, because 6 (in_size) != 2 (commandSize). But the second will

> > pass that check, because now in_size happens to match the commandSize

> > that has only been written to the buffer for the first command.

> 

> AFAIK tpm_transmit checks that the command has at least the header.


This was only a simple example, it will fail with other values as well. Just add 8 to both size fields and append 8 null bytes, and you will pass the length check in tpm_transmit but still have incorrect data. Also, it is probably no good style to omit checks for obvious errors, just because an unrelated check in a completely different location also happens to catch the problem under some circumstances. tpm_common_write discards the relevant information (in_size), so all other parts of the code need to be able to rely on it to have validated it properly.

> > 2. You may not reject commands where in_size > commandSize, because

> > TIS/PTP require the TPM to throw away extra bytes (and process the

> > command as usual), not fail the command. You can see that in the State

> > Transition Table (Table 18 in TIS 1.3), line 20, with the TPM in

> > Reception state and Expect=0, writing more data does not change the

> > state ("Write is not expected. Drop write. TPM ignores this state

> > transition."). Of course, since we do not pass on in_size, but only

> > commandSize the TPM will never see those extra bytes, but the external

> > behavior (for user space applications) is identical.

> 

> OK, this is more relevant. What is the legit case to send extra bytes?


For me: testing that my TPM implementation behaves correctly :) I can run the same test cases against the TPM using the Linux driver and several other, unrelated means. I'd like to avoid having special cases for communication paths in there, just because in one case I have a more direct channel to the TPM whereas in the other the Linux driver interferes with the communication and rejects the data before the TPM sees it. For "normal" usage from Linux applications this is not relevant, but it does not break anything either.

I've just noticed that the v2 patch is broken, because the code incorrectly tries to access data from user space. Interestingly, the tests worked fine on x86_64 and aarch64, only armv7l was broken (and that result somehow got lost, so I assumed that the patch was fine). I'll send a fixed patch soon.

Alexander
Jarkko Sakkinen Sept. 8, 2017, 9:34 p.m. UTC | #7
On Fri, Sep 08, 2017 at 02:26:42PM +0000, Alexander.Steffen@infineon.com wrote:
> > On Wed, Sep 06, 2017 at 02:19:28PM +0000,
> > Alexander.Steffen@infineon.com wrote:
> > > > On Wed, Sep 06, 2017 at 03:42:33PM +0300, Jarkko Sakkinen wrote:
> > > > > On Mon, Sep 04, 2017 at 07:36:42PM +0200, Alexander Steffen wrote:
> > > > > > tpm_transmit() does not offer an explicit interface to indicate the
> > > > > > number of valid bytes in the communication buffer. Instead, it
> > > > > > relies on the commandSize field in the TPM header that is encoded
> > within
> > > > the buffer.
> > > > > > Therefore, ensure that a) enough data has been written to the
> > > > > > buffer, so that the commandSize field is present and b) the
> > > > > > commandSize field does not announce more data than has been
> > written
> > > > to the buffer.
> > > > > >
> > > > > > This should have been fixed with CVE-2011-1161 long ago, but
> > > > > > apparently a correct version of that patch never made it into the
> > kernel.
> > > > > >
> > > > > > Cc: stable@vger.kernel.org
> > > > > > Signed-off-by: Alexander Steffen
> > <Alexander.Steffen@infineon.com>
> > > > > > ---
> > > > > > v2:
> > > > > > - Moved all changes to tpm_common_write in a single patch.
> > > > > >
> > > > > >  drivers/char/tpm/tpm-dev-common.c | 3 ++-
> > > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/char/tpm/tpm-dev-common.c
> > > > > > b/drivers/char/tpm/tpm-dev-common.c
> > > > > > index 610638a..ac25574 100644
> > > > > > --- a/drivers/char/tpm/tpm-dev-common.c
> > > > > > +++ b/drivers/char/tpm/tpm-dev-common.c
> > > > > > @@ -99,7 +99,8 @@ ssize_t tpm_common_write(struct file *file,
> > const
> > > > char __user *buf,
> > > > > >  	if (atomic_read(&priv->data_pending) != 0)
> > > > > >  		return -EBUSY;
> > > > > >
> > > > > > -	if (in_size > TPM_BUFSIZE)
> > > > > > +	if (in_size > sizeof(priv->data_buffer) || in_size < 6 ||
> > > > > > +	    in_size < be32_to_cpu(*((__be32 *) (buf + 2))))
> > > > > >  		return -E2BIG;
> > > > > >
> > > > > >  	mutex_lock(&priv->buffer_mutex);
> > > > > > --
> > > > > > 2.7.4
> > > > > >
> > > > >
> > > > > How did you test this change after you implemented it?
> > > > >
> > > > > Just thinking what to add to
> > > > > https://github.com/jsakkine-intel/tpm2-scripts
> > >
> > > I already had test cases that failed with some of my TPMs under some
> > circumstances. I'll try to come up with a concise description of what those
> > tests do or send you a patch directly for your tests. GitHub pull requests are
> > okay for that repository? (I already have one waiting there.)
> > >
> > > > >
> > > > > /Jarkko
> > > >
> > > > Just when I started to implement this that the bug fix itself does not have
> > yet
> > > > the right semantics.
> > > >
> > > > It should be just add a new check:
> > > >
> > > > if (in_size != be32_to_cpu(*((__be32 *) (buf + 2))))
> > > > 	return -EINVAL;
> > > >
> > > > The existing check is correct. This was missing. The reason for this is that
> > we
> > > > process whatever is in the in_size bytes as a full command.
> > >
> > > There are two problems with this solution:
> > >
> > > 1. You need to check for in_size < 6, otherwise you read data that has
> > > not been written there during that request. I haven't tested this, but
> > > I'd expect it to fail for example if you try to send the two commands
> > > "00 00 00 00 00 02" and "00 00". The first will be rejected with
> > > EINVAL, because 6 (in_size) != 2 (commandSize). But the second will
> > > pass that check, because now in_size happens to match the commandSize
> > > that has only been written to the buffer for the first command.
> > 
> > AFAIK tpm_transmit checks that the command has at least the header.
> 
> This was only a simple example, it will fail with other values as
> well. Just add 8 to both size fields and append 8 null bytes, and you
> will pass the length check in tpm_transmit but still have incorrect
> data. Also, it is probably no good style to omit checks for obvious
> errors, just because an unrelated check in a completely different
> location also happens to catch the problem under some circumstances.
> tpm_common_write discards the relevant information (in_size), so all
> other parts of the code need to be able to rely on it to have
> validated it properly.

I think any check should be done only in one place at the level where
it is required.

> > > 2. You may not reject commands where in_size > commandSize, because
> > > TIS/PTP require the TPM to throw away extra bytes (and process the
> > > command as usual), not fail the command. You can see that in the State
> > > Transition Table (Table 18 in TIS 1.3), line 20, with the TPM in
> > > Reception state and Expect=0, writing more data does not change the
> > > state ("Write is not expected. Drop write. TPM ignores this state
> > > transition."). Of course, since we do not pass on in_size, but only
> > > commandSize the TPM will never see those extra bytes, but the external
> > > behavior (for user space applications) is identical.
> > 
> > OK, this is more relevant. What is the legit case to send extra bytes?
> 
> For me: testing that my TPM implementation behaves correctly :) I can
> run the same test cases against the TPM using the Linux driver and
> several other, unrelated means. I'd like to avoid having special cases
> for communication paths in there, just because in one case I have a
> more direct channel to the TPM whereas in the other the Linux driver
> interferes with the communication and rejects the data before the TPM
> sees it. For "normal" usage from Linux applications this is not
> relevant, but it does not break anything either.
> 
> I've just noticed that the v2 patch is broken, because the code
> incorrectly tries to access data from user space. Interestingly, the
> tests worked fine on x86_64 and aarch64, only armv7l was broken (and
> that result somehow got lost, so I assumed that the patch was fine).
> I'll send a fixed patch soon.
> 
> Alexander

So what benefit do we get allowing garbage after the TPM command to
be sent? I think it makes more sense to allow only the command data
to be sent.

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/char/tpm/tpm-dev-common.c b/drivers/char/tpm/tpm-dev-common.c
index 610638a..ac25574 100644
--- a/drivers/char/tpm/tpm-dev-common.c
+++ b/drivers/char/tpm/tpm-dev-common.c
@@ -99,7 +99,8 @@  ssize_t tpm_common_write(struct file *file, const char __user *buf,
 	if (atomic_read(&priv->data_pending) != 0)
 		return -EBUSY;
 
-	if (in_size > TPM_BUFSIZE)
+	if (in_size > sizeof(priv->data_buffer) || in_size < 6 ||
+	    in_size < be32_to_cpu(*((__be32 *) (buf + 2))))
 		return -E2BIG;
 
 	mutex_lock(&priv->buffer_mutex);