[1/2] tpm/eventlog/tpm1: Simplify walking over *pos measurements
diff mbox series

Message ID 1547197173-52826-2-git-send-email-zhang.jia@linux.alibaba.com
State New
Headers show
Series
  • tpm/eventlog/tpm1: Small fixes
Related show

Commit Message

Jia Zhang Jan. 11, 2019, 8:59 a.m. UTC
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@linux.alibaba.com>
---
 drivers/char/tpm/eventlog/tpm1.c | 37 ++++++++++++++-----------------------
 1 file changed, 14 insertions(+), 23 deletions(-)

Comments

Jarkko Sakkinen Jan. 16, 2019, 10:09 p.m. UTC | #1
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@linux.alibaba.com>

What does this fix? Even if the current logic is "complicated", it is
still a pretty simple functiion.

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
Jia Zhang Jan. 17, 2019, 1:32 a.m. UTC | #2
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@linux.alibaba.com>
> 
> 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
>
Jarkko Sakkinen Jan. 18, 2019, 3:18 p.m. UTC | #3
On Thu, Jan 17, 2019 at 09:32:55AM +0800, Jia Zhang wrote:
> 
> 
> 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@linux.alibaba.com>
> > 
> > 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.

OK, got it now. I think I will apply this! Will take a while because
of https://lkml.org/lkml/2019/1/18/485. Will not apply new patches
before that is rooted.

/Jarkko
Jia Zhang Jan. 19, 2019, 7:48 a.m. UTC | #4
On 2019/1/18 下午11:18, Jarkko Sakkinen wrote:
> On Thu, Jan 17, 2019 at 09:32:55AM +0800, Jia Zhang wrote:
>>
>>
>> On 2019/1/17 上午6:09, Jarkko Sakkinen wrote:
>>> Please use "tpm:" tag for commits, not "tpm/eventlog/tpm1".
>>>

... snipped

> 
> OK, got it now. I think I will apply this! Will take a while because
> of https://lkml.org/lkml/2019/1/18/485. Will not apply new patches
> before that is rooted.

No problem. Thanks for your reviews.

Cheers,
Jia

> 
> /Jarkko
>

Patch
diff mbox series

diff --git a/drivers/char/tpm/eventlog/tpm1.c b/drivers/char/tpm/eventlog/tpm1.c
index 58c8478..4cf8303 100644
--- a/drivers/char/tpm/eventlog/tpm1.c
+++ b/drivers/char/tpm/eventlog/tpm1.c
@@ -74,7 +74,7 @@ 
 /* returns pointer to start of pos. entry of tcg log */
 static void *tpm1_bios_measurements_start(struct seq_file *m, loff_t *pos)
 {
-	loff_t i;
+	loff_t i = 0;
 	struct tpm_chip *chip = m->private;
 	struct tpm_bios_log *log = &chip->log;
 	void *addr = log->bios_event_log;
@@ -83,38 +83,29 @@  static void *tpm1_bios_measurements_start(struct seq_file *m, loff_t *pos)
 	u32 converted_event_size;
 	u32 converted_event_type;
 
-
 	/* read over *pos measurements */
-	for (i = 0; i < *pos; i++) {
+	do {
 		event = addr;
 
+		/* check if current entry is valid */
+		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 ((addr + sizeof(struct tcpa_event)) < limit) {
-			if ((converted_event_type == 0) &&
-			    (converted_event_size == 0))
-				return NULL;
-			addr += (sizeof(struct tcpa_event) +
-				 converted_event_size);
-		}
-	}
-
-	/* now check if current entry is valid */
-	if ((addr + sizeof(struct tcpa_event)) >= limit)
-		return NULL;
-
-	event = addr;
+		if (((converted_event_type == 0) && (converted_event_size == 0))
+		    || ((addr + sizeof(struct tcpa_event) + converted_event_size)
+			>= limit))
+			return NULL;
 
-	converted_event_size = do_endian_conversion(event->event_size);
-	converted_event_type = do_endian_conversion(event->event_type);
+		if (i++ == *pos)
+			break;
 
-	if (((converted_event_type == 0) && (converted_event_size == 0))
-	    || ((addr + sizeof(struct tcpa_event) + converted_event_size)
-		>= limit))
-		return NULL;
+		addr += (sizeof(struct tcpa_event) + converted_event_size);
+	} while (1);
 
 	return addr;
 }