Re: [PATCH 1/2] tpm/eventlog/tpm1: Simplify walking over *pos measurements

From: Jia Zhang
Date: Wed Jan 16 2019 - 21:35:36 EST




On 2019/1/17 äå6:09, Jarkko Sakkinen wrote:
> Please use "tpm:" tag for commits, not "tpm/eventlog/tpm1".
>
> On Fri, Jan 11, 2019 at 04:59:32PM +0800, Jia Zhang wrote:
>> The responsibility of tpm1_bios_measurements_start() is to walk
>> over the first *pos measurements, ensuring the skipped and
>> to-be-read measurements are not out-of-boundary.
>>
>> Current logic is complicated a bit. Just employ a do-while loop
>> with necessary sanity check, and then get the goal.
>>
>> Signed-off-by: Jia Zhang <zhang.jia@xxxxxxxxxxxxxxxxx>
>
> What does this fix? Even if the current logic is "complicated", it is
> still a pretty simple functiion.


OK. Let me point out the fix part. Here is the original implementation:

87 /* read over *pos measurements */
88 for (i = 0; i < *pos; i++) {
89 event = addr;
90
91 converted_event_size =
92 do_endian_conversion(event->event_size);
93 converted_event_type =
94 do_endian_conversion(event->event_type);
95
96 if ((addr + sizeof(struct tcpa_event)) < limit) {
97 if ((converted_event_type == 0) &&
98 (converted_event_size == 0))
99 return NULL;
100 addr += (sizeof(struct tcpa_event) +
101 converted_event_size);
102 }
103 }

The problem (just ignore all off-by-1 issues) is that accessing to
event_size and event_type is not pre-checked carefully. In the latter
part of tpm1_bios_measurements_start() and
tpm1_bios_measurements_next(), there is a fixed patter to do the sanity
check like this:

136 /* now check if current entry is valid */
137 if ((v + sizeof(struct tcpa_event)) >= limit)
138 return NULL;

So if we simply change this read-over chunk with sanity check like this:

/* read over *pos measurements */
for (i = 0; i < *pos; i++) {
event = addr;

if ((addr + sizeof(struct tcpa_event)) >= limit)
return NULL;

converted_event_size =
do_endian_conversion(event->event_size);
converted_event_type =
do_endian_conversion(event->event_type);

if ((converted_event_type == 0) &&
(converted_event_size == 0))
return NULL;
addr += (sizeof(struct tcpa_event) +
converted_event_size);
}

We will get two highly similar code chunks in
tpm1_bios_measurements_start(). Here is the latter part:

106 /* now check if current entry is valid */
107 if ((addr + sizeof(struct tcpa_event)) >= limit)
108 return NULL;
109
110 event = addr;
111
112 converted_event_size = do_endian_conversion(event->event_size);
113 converted_event_type = do_endian_conversion(event->event_type);
114
115 if (((converted_event_type == 0) && (converted_event_size == 0))
116 || ((addr + sizeof(struct tcpa_event) +
converted_event_size)
117 >= limit))
118 return NULL;
119
120 return addr;

So using a do while logic can simply merge them together and thus simply
and optimize the logic of walking over *pos measurements.

Sorry I admit my initial motivation is to fix up the sanity check
problem. If you would like to accept the optimization part, I will split
this patch.

Jia

>
> Applying clean ups for fun has the side-effect of making backporting
> more difficult. And swapping implementation randomly has the side-effect
> of potentially introducing regressions. The current code might be messy
> but it is still field tested.
>
> I'm sorry but I have to reject this patch.
>
> /Jarkko
>