linux-kernel - Re: [PATCH v7] tpm: Check size of response before accessing data

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <b80e79ee-8902-e82f-389b-feed075fe07a@linux.vnet.ibm.com>
Date:   Fri, 20 Jan 2017 16:04:14 -0500
From:   Stefan Berger <stefanb@...ux.vnet.ibm.com>
To:     Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
Cc:     tpmdd-devel@...ts.sourceforge.net,
        linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7] tpm: Check size of response before accessing data

On 01/20/2017 03:14 PM, Jarkko Sakkinen wrote:
> On Fri, Jan 20, 2017 at 07:55:22PM +0200, Jarkko Sakkinen wrote:
>> On Fri, Jan 20, 2017 at 06:21:58PM +0200, Jarkko Sakkinen wrote:
>>> On Fri, Jan 20, 2017 at 03:36:30PM +0200, Jarkko Sakkinen wrote:
>>>> On Thu, Jan 19, 2017 at 07:19:12AM -0500, Stefan Berger wrote:
>>>>> Make sure that we have not received less bytes than what is indicated
>>>>> in the header of the TPM response. Also, check the number of bytes in
>>>>> the response before accessing its data.
>>>>>
>>>>> Signed-off-by: Stefan Berger <stefanb@...ux.vnet.ibm.com>
>>>> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
>>> Oops. I found some odd stuff after all so hold on for a moment.
>>> I could do these updates myself probably...
>>>
>>>>>   ssize_t tpm_transmit_cmd(struct tpm_chip *chip, const void *cmd,
>>>>> -			 int len, unsigned int flags, const char *desc)
>>>>> +			 size_t len, size_t min_rsp_body_length,
>>>>> +			 unsigned int flags, const char *desc)
>>> BTW, maybe the cmd_length would be actually a better idea because
>>> it gets mixes witht local variable.
>>>
>>>>>   {
>>>>>   	const struct tpm_output_header *header;
>>>>>   	int err;
>>>>> +	ssize_t length;
>>> Maybe it would make sense to name this as rsp_length.
>>>
>>>>>   
>>>>> -	len = tpm_transmit(chip, (const u8 *)cmd, len, flags);
>>>>> -	if (len <  0)
>>>>> -		return len;
>>>>> -	else if (len < TPM_HEADER_SIZE)
>>>>> +	length = tpm_transmit(chip, (const u8 *)cmd, len, flags);
>>>>> +	if (length <  0)
>>>>> +		return length;
>>>>> +	else if (length < TPM_HEADER_SIZE)
>>>>>   		return -EFAULT;
>>>>>   
>>>>>   	header = cmd;
>>>>> +	if (length < be32_to_cpu(header->length))
>>>>> +		return -EFAULT;
>>> Why '<' and not '!='? In what legit case length would be larger?
>>>
>>>>>   
>>>>>   	err = be32_to_cpu(header->return_code);
>>>>>   	if (err != 0 && desc)
>>>>>   		dev_err(&chip->dev, "A TPM error (%d) occurred %s\n", err,
>>>>>   			desc);
>>>>> +	if (err)
>>>>> +		return err;
>>>>>   
>>>>> -	return err;
>>>>> +	if (be32_to_cpu(header->length) <
>>>>> +	    min_rsp_body_length + TPM_HEADER_SIZE)
>>>>> +		return -EFAULT;
>>> Why couldn't you use 'length' here?
>>>
>>> /Jarkko
>> Anyway,
>>
>> Tested-by: Jarkko Sakkinen <jarkko.sakkinen@...ux.intel.com>
> Stefan, I updated the patch by doing '!=' check and renaming parameters
> to 'buf' and 'bufsiz' as they are in tpm_transmit(). The current namesd
> did not make sense because you pass a buffer that will also will store
> the response.
>
> Can you check that after my updates it looks OK to you?

LGTM.

The != you introduced is correct (and stricter).