| Message ID | 1547197173-52826-2-git-send-email-zhang.jia@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tpm/eventlog/tpm1: Small fixes | expand |
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
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 >
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
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 >
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; }
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(-)