| Message ID | 20211116232407.291489-1-ilias.apalodimas@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | efi_loader: fix FinalEvent table initial position | expand |
Hi Heinrich, On Wed, 17 Nov 2021 at 01:24, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > When initializing the final event table in create_final_event() we are > setting the initial buffer position to sizeof(*final_event). Although > we clear the buffer correctly and won't cause any problems, we should > start logging events starting from zero. > While at it add a cast when defining the final_event. > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > lib/efi_loader/efi_tcg2.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c > index 189e4a5ba59c..634556342a7c 100644 > --- a/lib/efi_loader/efi_tcg2.c > +++ b/lib/efi_loader/efi_tcg2.c > @@ -1257,10 +1257,11 @@ static efi_status_t create_final_event(void) > goto out; > > memset(event_log.final_buffer, 0xff, TPM2_EVENT_LOG_SIZE); > - final_event = event_log.final_buffer; > + final_event = > + (struct efi_tcg2_final_events_table *)event_log.final_buffer; > final_event->number_of_events = 0; > final_event->version = EFI_TCG2_FINAL_EVENTS_TABLE_VERSION; > - event_log.final_pos = sizeof(*final_event); > + event_log.final_pos = 0; > ret = efi_install_configuration_table(&efi_guid_final_events, > final_event); > if (ret != EFI_SUCCESS) { > -- > 2.33.1 > After sending this I just realized it's wrong. The current code is correct, since we have to account for the number of events and version Sorry for the noise Cheers /Ilias
On 11/17/21 00:24, Ilias Apalodimas wrote: > When initializing the final event table in create_final_event() we are > setting the initial buffer position to sizeof(*final_event). Although > we clear the buffer correctly and won't cause any problems, we should > start logging events starting from zero. > While at it add a cast when defining the final_event. > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > --- > lib/efi_loader/efi_tcg2.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c > index 189e4a5ba59c..634556342a7c 100644 > --- a/lib/efi_loader/efi_tcg2.c > +++ b/lib/efi_loader/efi_tcg2.c > @@ -1257,10 +1257,11 @@ static efi_status_t create_final_event(void) > goto out; > > memset(event_log.final_buffer, 0xff, TPM2_EVENT_LOG_SIZE); > - final_event = event_log.final_buffer; > + final_event = > + (struct efi_tcg2_final_events_table *)event_log.final_buffer; final_buffer is defined as void *. So this cast is superfluous and should be avoided. Cf. https://www.kernel.org/doc/html/latest/process/coding-style.html#allocating-memory There is a further unneeded conversion of the same type in the file. Why didn't you define component final_buffer as struct efi_tcg2_final_events_table * and just added a void ** conversion to the efi_allocate_pool() call? This would allow to eliminate the variable final_event here and in tcg2_agile_log_append(). Best regards Heinrich > final_event->number_of_events = 0; > final_event->version = EFI_TCG2_FINAL_EVENTS_TABLE_VERSION; > - event_log.final_pos = sizeof(*final_event); > + event_log.final_pos = 0; > ret = efi_install_configuration_table(&efi_guid_final_events, > final_event); > if (ret != EFI_SUCCESS) { >
On Wed, 17 Nov 2021 at 11:10, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 11/17/21 00:24, Ilias Apalodimas wrote: > > When initializing the final event table in create_final_event() we are > > setting the initial buffer position to sizeof(*final_event). Although > > we clear the buffer correctly and won't cause any problems, we should > > start logging events starting from zero. > > While at it add a cast when defining the final_event. > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > --- > > lib/efi_loader/efi_tcg2.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c > > index 189e4a5ba59c..634556342a7c 100644 > > --- a/lib/efi_loader/efi_tcg2.c > > +++ b/lib/efi_loader/efi_tcg2.c > > @@ -1257,10 +1257,11 @@ static efi_status_t create_final_event(void) > > goto out; > > > > memset(event_log.final_buffer, 0xff, TPM2_EVENT_LOG_SIZE); > > - final_event = event_log.final_buffer; > > + final_event = > > + (struct efi_tcg2_final_events_table *)event_log.final_buffer; > > final_buffer is defined as void *. So this cast is superfluous and > should be avoided. Cf. > https://www.kernel.org/doc/html/latest/process/coding-style.html#allocating-memory > > There is a further unneeded conversion of the same type in the file. > > Why didn't you define component final_buffer as struct > efi_tcg2_final_events_table * and just added a void ** conversion to the > efi_allocate_pool() call? > > This would allow to eliminate the variable final_event here and in > tcg2_agile_log_append(). Because both the buffer and the final_buffer describe log events of different type that need to be appended on the eventlog using the same functions. Due to all the ptr arithmetic that takes place, I though having them as void would be easier to read. In any case this patch shouldn't be applied. Cheers /Ilias > Best regards > > Heinrich > > > final_event->number_of_events = 0; > > final_event->version = EFI_TCG2_FINAL_EVENTS_TABLE_VERSION; > > - event_log.final_pos = sizeof(*final_event); > > + event_log.final_pos = 0; > > ret = efi_install_configuration_table(&efi_guid_final_events, > > final_event); > > if (ret != EFI_SUCCESS) { > > > >
diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c index 189e4a5ba59c..634556342a7c 100644 --- a/lib/efi_loader/efi_tcg2.c +++ b/lib/efi_loader/efi_tcg2.c @@ -1257,10 +1257,11 @@ static efi_status_t create_final_event(void) goto out; memset(event_log.final_buffer, 0xff, TPM2_EVENT_LOG_SIZE); - final_event = event_log.final_buffer; + final_event = + (struct efi_tcg2_final_events_table *)event_log.final_buffer; final_event->number_of_events = 0; final_event->version = EFI_TCG2_FINAL_EVENTS_TABLE_VERSION; - event_log.final_pos = sizeof(*final_event); + event_log.final_pos = 0; ret = efi_install_configuration_table(&efi_guid_final_events, final_event); if (ret != EFI_SUCCESS) {
When initializing the final event table in create_final_event() we are setting the initial buffer position to sizeof(*final_event). Although we clear the buffer correctly and won't cause any problems, we should start logging events starting from zero. While at it add a cast when defining the final_event. Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org> --- lib/efi_loader/efi_tcg2.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)