Currently, it is very easy to make the AMD microcode update driver crash
or spin on a malformed microcode container file since it does very little
consistency checking on data loaded from such file.
This series introduces various checks, mostly on length-type fields,
so all corrupted microcode container files are (hopefully) correctly
rejected instead.
This largely matches what the Intel microcode update driver already does.
It also tries to make the behavior consistent between the early and late
loaders.
Please note that this isn't about verifying the actual microcode update,
that is, the blob that gets sent to the CPU as the new microcode.
Such verification is (hopefully) done by the CPU itself.
It is about verifying a driver-specific container file that includes
many microcode updates for different CPUs of a particular CPU family,
along with metadata that helps the driver select the right microcode
update to actually send to the CPU.
There are purposely-corrupted test files available at
https://pastebin.com/XkKUSmMp
One has to enable KASAN in the kernel config and rename a particular
test file to name appropriate to the running CPU family to test its
loading.
Changes from v1:
* Capitalize a comment,
* Rename 'eqsize' and 'bufsize' to 'eq_size' and 'buf_size',
respectively,
* Attach a comment about checking the equivalence table header to its
first size check,
* Rename 'equiv{_cpu,}_table_size' to 'equiv{_cpu,}_table_entries'.
Changes from v2:
* Split the patch into separate commits,
* Remove explicit CPU equivalence table size limit,
* Make install_equiv_cpu_table() return a size_t instead of a (signed)
int so no overflow can occur there,
* Automatically compute the PATCH_MAX_SIZE macro and use it for checking
a patch size,
* Make the late loader behavior with respect to late parse failures
consistent with what the early loader does.
Changes from v3:
* Capitalize patch subject names,
* Add a comment describing why we subtract SECTION_HDR_SIZE from a file
leftover length before calling verify_patch_size(),
* Don't break a long line containing the above subtraction,
* Move a comment in parse_container() back to where it was in the original
patch version,
* Add special local vars for container header fields in parse_container(),
* Return the remaining blob size from parse_container() if the equivalence
table is truncated,
* Split the equivalence table size checks into two patches: one for the
early loader and one for the late loader,
* Rename an equivalence table length variable in install_equiv_cpu_table()
for consistency with a similar one in parse_container(),
* Rephrase the error messages in install_equiv_cpu_table() to be more
descriptive and merge two tests there so they print the same message,
* Change install_equiv_cpu_table() to return an unsigned int while moving
the job of adding the container header length to this value to its caller,
* Drop automatic computation of the PATCH_MAX_SIZE macro and add a comment
reminding to do it manually instead,
* Add a local variable patch_type for better code readability in
verify_and_add_patch() function.
arch/x86/include/asm/microcode_amd.h | 1 +
arch/x86/kernel/cpu/microcode/amd.c | 179 ++++++++++++++++++++++++-----------
2 files changed, 127 insertions(+), 53 deletions(-)
On Fri, Mar 16, 2018 at 12:07:34AM +0100, Maciej S. Szmigiero wrote:
> Currently, it is very easy to make the AMD microcode update driver crash
> or spin on a malformed microcode container file since it does very little
> consistency checking on data loaded from such file.
Ok, a couple of tips for the next time:
* please send your reworked patchset for review after review of your
current patchset has been completed. We normally send patchsets once a
week. You've sent yours two days after and that's kinda too fast. I'm
sure you can imagine reviewers have other work to do too.
So please be patient before resending next time.
If in doubt, the whole process is documented in Documentation/process/ -
might wanna skim through it.
* when sending your patchset, make sure all mails are sent as a reply to
the 0/N message.
Your 0/N has:
Message-ID: <[email protected]>
but your 1/N has
References: <[email protected]>
so something went wrong there. Normally git send-email does this
correctly.
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On 16.03.2018 00:23, Borislav Petkov wrote:
> On Fri, Mar 16, 2018 at 12:07:34AM +0100, Maciej S. Szmigiero wrote:
>> Currently, it is very easy to make the AMD microcode update driver crash
>> or spin on a malformed microcode container file since it does very little
>> consistency checking on data loaded from such file.
>
> Ok, a couple of tips for the next time:
>
> * please send your reworked patchset for review after review of your
> current patchset has been completed. We normally send patchsets once a
> week. You've sent yours two days after and that's kinda too fast. I'm
> sure you can imagine reviewers have other work to do too.
>
> So please be patient before resending next time.
>
> If in doubt, the whole process is documented in Documentation/process/ -
> might wanna skim through it.
I thought the one week absolute minimum resend time is for the case that
there are no comments to a patch, or, more generally, when there is no
action and not for "normal process" respins.
submitting-patches.rst says:
> Once upon a time, patches used to disappear into the void without comment,
> but the development process works more smoothly than that now. You should
> receive comments within a week or so; if that does not happen, make sure
> that you have sent your patches to the right place. Wait for a minimum of
> one week before resubmitting or pinging reviewers - possibly longer during
> busy times like merge windows.
But I will wait a week now for respins if you say so.
> * when sending your patchset, make sure all mails are sent as a reply to
> the 0/N message.
>
> Your 0/N has:
>
> Message-ID: <[email protected]>
>
> but your 1/N has
>
> References: <[email protected]>
>
> so something went wrong there. Normally git send-email does this
> correctly.
I've copied these messages to my mailbox first and sent them from there,
so that must have overwritten the message IDs.
Will submit them from git-send-email the next time.
> Thx.
Thanks,
Maciej
On Fri, Mar 16, 2018 at 12:40:40AM +0100, Maciej S. Szmigiero wrote:
> I thought the one week absolute minimum resend time is for the case that
> there are no comments to a patch, or, more generally, when there is no
> action and not for "normal process" respins.
>
> submitting-patches.rst says:
> > Once upon a time, patches used to disappear into the void without comment,
> > but the development process works more smoothly than that now. You should
> > receive comments within a week or so;
Well, it says that *exactly* - you should receive comments within a
week. In your case, you got some of them even earlier.
Think of it this way: one week is more or less ok period when reviewers
have time to go through a patchset and give their comments. Keep in mind
that maintainers/reviewers get other patchsets to review too and they
can't always react immediately. In *addition* to the other crap they
have on their plate.
So patience is a good idea here :)
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.