Re: [PATCH 2/2] efi/cper: Avoid possible OOB when checking generic data block

[Ross Lagerwall]

On 1/23/19 11:54 AM, Borislav Petkov wrote:
> On Tue, Jan 22, 2019 at 04:09:12PM +0000, Ross Lagerwall wrote:
> > When checking a generic status block, we iterate over all the generic
> > data blocks. The loop condition only checks that the start of the
> > generic data block is valid (within estatus->data_length) but not the
> > whole block. Because the size of data blocks (excluding error data) may
> > vary depending on the revision and the revision is contained within the
> > data block, ensure that enough of the current data block is valid before
> > dereferencing any members otherwise an OOB access may occur if
snip
> > -		data_len -= acpi_hest_get_record_size(gdata);
> > +		record_len = acpi_hest_get_record_size(gdata);
>
> record_size so that it matches the function name it is used to compute
> this.
>
> Btw, trying to grok this code is making my head spin.
>
> > +		if (record_len > data_len)
> > +			return -EINVAL;
>
> <---- newline here.
>
> Btw, those checks in the loop you can abstract away into a separate
> function so that you end up with something more readable like:
>
>          apei_estatus_for_each_section(estatus, gdata) {
>                  record_size = check_hest_record_size(gdata, data_len);
>                  if (!record_size)
>                          return -EINVAL;
>
>                  data_len -= record_size;
>          }
>
> for example.

There are only two if statements in the loop body -- I don't think it is 
necessary to abstract this into a separate function (which still 
requires having one if statement in the loop body).

I've made the other changes you suggested and sent a V2.

Thanks,
--
Ross Lagerwall