linux-kernel - Re: [PATCH v5] ACPI / APEI: fix the regression of synchronous external aborts occur in user-mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4e1416f4-7556-8885-6fa5-bdf8336ae9f6@huawei.com>
Date:   Tue, 8 Jun 2021 17:15:05 +0800
From:   Xiaofei Tan <tanxiaofei@...wei.com>
To:     James Morse <james.morse@....com>, <rafael@...nel.org>,
        <rjw@...ysocki.net>, <lenb@...nel.org>, <tony.luck@...el.com>,
        <bp@...en8.de>, <akpm@...ux-foundation.org>, <jroedel@...e.de>,
        <peterz@...radead.org>
CC:     <linux-acpi@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linuxarm@...wei.com>
Subject: Re: [PATCH v5] ACPI / APEI: fix the regression of synchronous
 external aborts occur in user-mode


Hi James,

On 2021/6/5 16:59, Xiaofei Tan wrote:
> Hi James,
>
> On 2021/6/4 22:19, James Morse wrote:

...

>>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>>> index fce7ade..0893968 100644
>>> --- a/drivers/acpi/apei/ghes.c
>>> +++ b/drivers/acpi/apei/ghes.c
>>
>>> +static bool ghes_handle_arm_hw_error(struct acpi_hest_generic_data *gdata, int sev)
>>> +{
>>> +    struct cper_sec_proc_arm *err = acpi_hest_get_payload(gdata);
>>> +    struct cper_arm_err_info *err_info;
>>> +    bool queued = false;
>>> +    int sec_sev, i;
>>> +
>>> +    log_arm_hw_error(err);
>>> +
>>> +    sec_sev = ghes_severity(gdata->error_severity);
>>> +    if (sev != GHES_SEV_RECOVERABLE || sec_sev != GHES_SEV_RECOVERABLE)
>>> +        return false;
>>> +
>>> +    err_info = (struct cper_arm_err_info *) (err + 1);
>>> +    for (i = 0; i < err->err_info_num; i++, err_info++) {
>>
>> err_info has a version and a length, so its expected to be made bigger at some point.
>
>
> Yes, but the table "ARM Processor Error Section" fixed the length of
> processor error information structure to fixed 32 bytes. Then the
> description of the UEFI spec need to update.
>
>> It would be better to use the length instead of 'err_info++', or at least to break out of
>> the loop if a length > sizeof(*err_info) is seen.
>>
>
> OK. Maybe check length != sizeof(*err_info) is better. I will add this.
>
>     if (err_info->length != sizeof(struct cper_arm_err_info)) {
>         pr_warn_ratelimited(FW_WARN GHES_PFX
>                     "Error info length %d is invalid\n",
>                     err_info->length);
>         break;
>     }
>

I considered this more carefully. It should be better to use the length instead of 'err_info', than
just break out. Because if we want to expand "ARM Processor Error Infomation Structure", the proper way
is just add some new member at the end of the structure. Then we have a chance to work well for the situation
of new firmware + old kernel.

>> With that:
>> Reviewed-by: James Morse <james.morse@....com>
>>
>>
>> The following nits would make this easier to read:
>>



>>
>
> Right.
>
>>
>> Thanks,
>>
>> James
>>
>> .
>>
>
>
> .
>