Re: [tpmdd-devel] char/tpm: CVE-2011-1161 and CVE-2011-1162

[Eugene T. <eug...@ke...>]

On Fri, Sep 9, 2011 at 3:02 PM, Peter Huewe
<hue...@go...> wrote:
> On Thu, Sep 8, 2011 at 8:38 PM, Rajiv Andrade <sr...@li...> wrote:
>> Hi Jiri,
>>
>> Thanks for bringing these patches up to discussion again:
>>
>> On 08-09-2011 07:43, Jiri Benc wrote:
>>> Hi,
>>>
>>> in March, there were three security related patches posted to LKML:
>>> https://lkml.org/lkml/2011/3/16/479
>>>
>>> The first one got to Linus' tree, while the other two did not. For
>>> reference, I'm including them here (reconstructed from the mentioned
>>> thread as the git tree disappeared - they're probably missing part
>>> of their original descriptions).
>>>
>>> ------
>>>
>>> From: Peter Huewe <hue...@go...>
>>> Subject: char/tpm: Call tpm_transmit with correct size
>>>
>>> This patch changes the call of tpm_transmit by supplying the size of the
>>> userspace buffer instead of TPM_BUFSIZE.
>>>
>>> This got assigned CVE-2011-1161.
>>>
>>> --- a/drivers/char/tpm/tpm.c
>>> +++ b/drivers/char/tpm/tpm.c
>>> @@ -427,7 +427,7 @@ static ssize_t tpm_transmit(struct tpm_c
>>>       goto out;
>>>
>>>  out_recv:
>>> -     rc = chip->vendor.recv(chip, (u8 *) buf, bufsiz);
>>> +     rc = chip->vendor.recv(chip, (u8 *) buf, TPM_BUFSIZE);
>>>       if (rc < 0)
>>>               dev_err(chip->dev,
>>>                       "tpm_transmit: tpm_recv: error %zd\n", rc);
>>> @@ -1085,7 +1085,7 @@ ssize_t tpm_write(struct file *file, con
>>>       }
>>>
>>>       /* atomic tpm command send and result receive */
>>> -     out_size = tpm_transmit(chip, chip->data_buffer, TPM_BUFSIZE);
>>> +     out_size = tpm_transmit(chip, chip->data_buffer, in_size);
>>>
>>>       atomic_set(&chip->data_pending, out_size);
>>>       mutex_unlock(&chip->buffer_mutex);
>>>
>>> ------
>>>
>>> From: Peter Huewe <hue...@go...>
>>> Subject: char/tpm: Zero buffer after copying to userspace
>>>
>>> Since the buffer might contain security related data it might be a good idea to
>>> zero the buffer after we have copied it to userspace.
>>>
>>> This got assigned CVE-2011-1162.
>>>
>>> --- a/drivers/char/tpm/tpm.c
>>> +++ b/drivers/char/tpm/tpm.c
>>> @@ -1102,6 +1102,7 @@ ssize_t tpm_read(struct file *file, char
>>>  {
>>>       struct tpm_chip *chip = file->private_data;
>>>       ssize_t ret_size;
>>> +     int rc;
>>>
>>>       del_singleshot_timer_sync(&chip->user_read_timer);
>>>       flush_work_sync(&chip->work);
>>> @@ -1112,8 +1113,11 @@ ssize_t tpm_read(struct file *file, char
>>>                       ret_size = size;
>>>
>>>               mutex_lock(&chip->buffer_mutex);
>>> -             if (copy_to_user(buf, chip->data_buffer, ret_size))
>>> +             rc = copy_to_user(buf, chip->data_buffer, ret_size);
>>> +             memset(chip->data_buffer, 0, ret_size);
>>> +             if (rc)
>>>                       ret_size = -EFAULT;
>>> +
>>>               mutex_unlock(&chip->buffer_mutex);
>>>       }
>>>
>>> ------
>>>
>>> What's the status of those patches?
>> Some were under discussion IIRC.
>>> The first patch looks a bit suspicious to me, btw, as tpm_transmit is
>>> called from at least one place with a small static buffer
>>> (tpm_continue_selftest), receiving into such buffer up to TPM_BUFSIZE
>>> bytes doesn't look correct. I think the second hunk of the patch alone
>>> should be enough, as in_size has been modified to be <= TPM_BUFSIZE
>>> earlier in tpm_write.
>>
>> Ok, I remember now.
>>
>> I agree, tpm_transmit's last parameter is somewhat misleading, since it means the amount
>> of data that is expected from the TPM, although it indeed acts as a boundary check as well.
>>
>> The check then should really be if that amount of expected data can fit into *buf, before the
>> point that hunk touches, I'll rewrite it.
>>
>> This version of the second patch is fine.
>
>
> Rajiv,
>> I'll rewrite it.
> Great! I'm really looking forward to see this sorted out ;) If you
> want I can probably review/test it.
>
>> This version of the second patch is fine.
> Should I resent the second patch or do you have it in your queue?
>
>
> Added Eugene Theo and Moritz Muehlenhoff (of Debian) to the
> discussion, as they both are interested in the status of these patches
> too.

Did you relocate the git trees to elsewhere since git.kernel.org is
down atm? Any update with the patches? Please keep me in the loop.
Appreciated!

Thanks, Eugene