linux-kernel - Re: [PATCH v1 1/3] firmware: coreboot: support for parsing SMM related informations from coreboot tables

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 for Android: free password hash cracker in your pocket

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

Message-ID: <aE1wvuamXqVTzpny@cyber-t14sg4>
Date: Sat, 14 Jun 2025 14:53:18 +0200
From: Michal Gorlas <michal.gorlas@...ements.com>
To: Brian Norris <briannorris@...omium.org>
Cc: Tzung-Bi Shih <tzungbi@...nel.org>,
	Julius Werner <jwerner@...omium.org>, marcello.bauer@...ements.com,
	chrome-platform@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/3] firmware: coreboot: support for parsing SMM
 related informations from coreboot tables

Hi,
Thanks for taking your time to go over the patches.

On Thu, Jun 12, 2025 at 03:37:40PM -0700, Brian Norris wrote:
> 
> This all looks highly X86-specific, so you probably need 'depends on
> X86'.
> 

Indeed, this was also why CI build was failed.

> > @@ -112,8 +122,8 @@ void coreboot_driver_unregister(struct coreboot_driver *driver);
> >   * boilerplate.  Each module may only use this macro once, and
> >   * calling it replaces module_init() and module_exit()
> >   */
> > -#define module_coreboot_driver(__coreboot_driver) \
> > +#define module_coreboot_driver(__coreboot_driver)                  \
> >  	module_driver(__coreboot_driver, coreboot_driver_register, \
> > -			coreboot_driver_unregister)
> > +		      coreboot_driver_unregister)
> 
> You're making arbitrary whitespace changes in this hunk. Try to avoid
> that, please.
> 

Sure, will do. It came from a style warning when running
scripts/checkpatch.pl. I thought it could be useful to fix it on the
same go.

> >  
> >  #endif /* __COREBOOT_TABLE_H */
> > diff --git a/drivers/firmware/google/mm_info.c b/drivers/firmware/google/mm_info.c
> > new file mode 100644
> > index 000000000000..55bcdc8b8d53
> > --- /dev/null
> > +++ b/drivers/firmware/google/mm_info.c
> > @@ -0,0 +1,63 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * mm_info.c
> > + *
> > + * Driver for exporting MM payload information from coreboot table.
> > + *
> > + * Copyright 2025 9elements gmbh
> > + * Copyright 2025 Michal Gorlas <michal.gorlas@...ements.com>
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +
> > +#include "coreboot_table.h"
> > +#include "mm_payload.h"
> > +
> > +static struct lb_pld_mm_interface_info *mm_cbtable_info;
> > +struct mm_info *mm_info;
> > +
> > +static int mm_driver_probe(struct coreboot_device *dev)
> > +{
> > +	mm_cbtable_info = &dev->mm_info;
> > +	if (mm_cbtable_info->tag != LB_TAG_PLD_MM_INTERFACE_INFO)
> > +		return -ENXIO;
> > +
> > +	mm_info = kmalloc(sizeof(*mm_info), GFP_KERNEL);
> 
> Error handling? (Needs a NULL check.)
> 
> And might as well use devm_*() (e.g., devm_kzalloc()); then you can drop
> mm_driver_remove().
> 
> > +	mm_info->revision = mm_cbtable_info->revision;
> > +	mm_info->requires_long_mode_call =
> > +		mm_cbtable_info->requires_long_mode_call;
> > +	mm_info->register_mm_entry_command =
> > +		mm_cbtable_info->register_mm_entry_command;
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(mm_info);
> 
> Why non-GPL? IIUC, EXPORT_SYMBOL_GPL() is the usual default.
> 
> Or, I suspect you don't actually need two separate modules, so you
> probably don't need this EXPORT at all.
> 

Yep these two definetely can be merged into one module. Will do that in
v2 series.

Best,
Michal