flashrom: Add a plugin for updating using the flashrom command line tool #295

hughsie · 2017-11-03T09:32:14Z

This reworks the old purism plugin to be a generic flashrom mechanism that can be used by any vendor by dropping in a quirks file.

superm1

As a more general comment, you should add flashrom and bubblewrap to the dependencies.txt for all the distro packages.

superm1 · 2017-11-03T19:54:52Z

plugins/flashrom/example/com.Flashrom.Laptop.metainfo.xml

+<?xml version="1.0" encoding="UTF-8"?>
+<!-- Copyright 2017 Richard Hughes <richard@hughsie.com> -->
+<component type="firmware">
+  <id>com.Flashrom.Laptop.firmware</id>


Any particular reason to capitalize Laptop and Flashrom?

It's just example text really.

superm1 · 2017-11-03T19:55:25Z

plugins/flashrom/example/com.Flashrom.Laptop.metainfo.xml

+    </p>
+  </description>
+  <provides>
+    <firmware type="flashed">a0ce5085-2dea-5086-ae72-45810a186ad0</firmware>


A comment on how this GUID was generate would be good

superm1 · 2017-11-03T19:56:28Z

plugins/flashrom/example/com.Flashrom.Laptop.metainfo.xml

+
+  <!-- only newer versions of fwupd know how to write to this hardware -->
+  <requires>
+    <id compare="ge" version="0.9.7">org.freedesktop.fwupd</id>


This should probably match 1.x.x as that's when the plugin will merge in.

superm1 · 2017-11-03T20:00:14Z

plugins/flashrom/fu-plugin-flashrom.c

+		fu_plugin_set_status (plugin, FWUPD_STATUS_DEVICE_VERIFY);
+	//FIXME get percentage completion...
+	g_debug ("got '%s'", line);
+}


Obviously sort this out before merging

I'm actually waiting for hardware to know how to parse this; the dummy flashrom driver I've been using doesn't return percentage reports. :/ I've asked on the flashrom mailing list about the creation of a libflashrom which would be much better to use, but such a think might be a long time waiting.

@ximion can I get hold of the flashrom --verbose output please so I know what to parse please. If the Purism guys can get some hardware to me (even just a reconditioned mainboard) then that would make this much easier to test.

I've guessed at this now, confirmation required of course.

superm1 · 2017-11-03T20:00:20Z

plugins/flashrom/fu-plugin-flashrom.c

+	if (g_strcmp0 (line, "Writing flash...") == 0)
+		fu_plugin_set_status (plugin, FWUPD_STATUS_DEVICE_WRITE);
+	//FIXME get percentage completion...
+	g_debug ("got '%s'", line);


superm1 · 2017-11-03T20:20:43Z

plugins/flashrom/fu-plugin-flashrom.c

+		  GError **error)
+{
+	FuPluginData *data = fu_plugin_get_data (plugin);
+	g_autofree gchar *flashrom_fn = NULL;


unused AFAICT

superm1 · 2017-11-03T20:21:14Z

plugins/flashrom/fu-plugin-flashrom.c

+	g_autofree gchar *firmware_fn = NULL;
+	g_autofree gchar *tmpdir = NULL;
+	const gchar *argv[] = {
+		"/xxx/flashrom",


Can this directly be initialized to data->flashrom_fn?

superm1 · 2017-11-03T20:23:52Z

src/fu-quirks.h

+ *
+ * When the system motherboard can be updated using flashrom, this quirk maps
+ * a hardware ID to a device ID.
+ *


Flashrom can be used for more than just system motherboards can't it? It might make sense to be more general in this regard. For example add a quirk that represents system motherboard, and then a different one that represents some other random device (not sure what arguments flashrom uses to represent access to something other than internal:laptop though).

I've never seen flashrom update anything other than the BIOS in the wild -- I know it can do things like network cards, but I'm really pushing vendors to use UEFI for that kind of thing -- flashrom is a last ditch effort really.

OK probably fine to keep it this way and if the need arises it can be changed some then.

superm1 · 2017-11-03T20:27:37Z

plugins/flashrom/example/com.Flashrom.Laptop.metainfo.xml

+  <custom>
+    <value key="fwupd::BuilderScript">startup.sh</value>
+    <value key="fwupd::BuilderOutput">firmware.bin</value>
+  </custom>


It's not obvious that these are called coming into the update function. I think you should add something in the readme or at least gtk-doc documentation for the flashrom plugin to indicate that's how this is working.

there is already documentation in (the installed) data/builder/README.md but I can add something to the readme.md too

superm1 · 2017-11-03T20:32:17Z

plugins/meson.build

@@ -1,5 +1,6 @@
 subdir('dfu')
 subdir('ebitdo')
+subdir('flashrom')


I think it would be good to make this plugin optional by meson config option (but default on). Eg thinking with a RHEL hat on, I'm not sure it's something that should be on RHEL without an explicit ask from a customer.

What's the logic for not including it in RHEL? Perhaps that the flashrom update mechanism is inherently riskier than a UEFI capsule update?

Yeah inherently riskier is exactly what I was getting at. There's no validation of the signature of a payload.

Right, agree. Is flashing an unsigned system firmware from the LVFS more risky than not applying a security update? My argument would be you remove the layer of hardware validation, and fall back to just the two layers of LVFS+fwupd policy.

I suppose and if flashrom the runtime dep isn't installed by deafult yet this is an extremely small surface area.

hughsie · 2017-11-04T22:06:57Z

you should add flashrom and bubblewrap to the dependencies.txt

These are runtime deps tho, not BuildRequires...

superm1 · 2017-11-04T22:12:34Z

Yeah so debian/control and spec.in will both want the runtime deps included then too

hughsie · 2017-11-05T20:22:28Z

How do you do a runtime dep in Debian out of interest? Google wasn't much help. For Fedora I've just added bubblewrap, I don't know if a 600Kb dep of flashrom is what we want in general. I suppose if we "support" it in fwupd then we do need to runtime require it. I'd still much rather depend on a libflashrom-dev although that might be some waiting.

superm1 · 2017-11-06T15:41:58Z

How do you do a runtime dep in Debian out of interest?
debian/control add it to the depends: or recommends: line of the package that needs the runtime dep or recommend. So here: https://github.com/hughsie/fwupd/blob/master/contrib/debian/control#L78

Since these are "optional", I think it makes sense to mark bubblewrap as recommends. Recommends are installed by default, but they can be removed and let the package still be functional.

Keeping flashrom out for now sounds fine by me. As these machines are prevalent that use it that can be revisited. I think If it's not installed by default though you should make a few changes:

Rather than abort the plugin if not installed, only avoid adding the "updateable" flag.
Output something in the journal (error or info level) about missing flashrom executable in this instance if one of these devices is found.
Add something to the common errors wiki about this error with a fix for common distros (apt install flashrom; dnf install flashrom)

In ubuntu land there will be some paper work with getting the various deps installed by default (in the regular public image), but I'll sort that out.

superm1 · 2018-08-27T16:25:59Z

plugins/flashrom/fu-plugin-flashrom.c

+				     FWUPD_ERROR,
+				     FWUPD_ERROR_NOT_SUPPORTED,
+				     "flashrom is not installed");
+		return FALSE;


Since we've been switching over to marking devices not updatable and setting UpdateError, I think this is a good candidate to do the same rather than fail plugin startup.

Agree. I'm still not sure if I want to merge this plugin or wait for libflashrom to happen. It doesn't seem like the latter is coming any time soon.

I say update for that change merge it now, and if libflashrom happens, clean up later, bump up the dependency.

hughsie requested a review from superm1 November 3, 2017 09:32

superm1 reviewed Nov 3, 2017

View reviewed changes

superm1 mentioned this pull request Nov 3, 2017

Generate debian/control dynamically #287

Merged

hughsie force-pushed the wip/hughsie/purism branch from b88f03f to 1bf1636 Compare November 4, 2017 22:12

hughsie force-pushed the wip/hughsie/purism branch from 1bf1636 to 1cbfa7f Compare December 1, 2017 13:54

hughsie force-pushed the wip/hughsie/purism branch from 1cbfa7f to cc5a8ed Compare April 3, 2018 18:04

hughsie force-pushed the wip/hughsie/purism branch from cc5a8ed to 8ab897b Compare May 14, 2018 19:45

hughsie force-pushed the wip/hughsie/purism branch from 8ab897b to ae31dfb Compare May 25, 2018 15:41

hughsie force-pushed the wip/hughsie/purism branch from ae31dfb to 9d5d1e1 Compare July 3, 2018 09:04

hughsie force-pushed the wip/hughsie/purism branch from 9d5d1e1 to 95c2a59 Compare August 14, 2018 09:42

hughsie force-pushed the wip/hughsie/purism branch 2 times, most recently from e9183b7 to 88db1d4 Compare August 26, 2018 20:44

superm1 reviewed Aug 27, 2018

View reviewed changes

hughsie force-pushed the wip/hughsie/purism branch from 88db1d4 to 8d0c3c0 Compare August 27, 2018 18:16

flashrom: Add a plugin for updating using the flashrom command line tool

e52c489

hughsie force-pushed the wip/hughsie/purism branch from 8d0c3c0 to e52c489 Compare August 28, 2018 08:25

hughsie merged commit c165542 into master Aug 28, 2018

hughsie deleted the wip/hughsie/purism branch August 28, 2018 08:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

flashrom: Add a plugin for updating using the flashrom command line tool #295

flashrom: Add a plugin for updating using the flashrom command line tool #295

hughsie commented Nov 3, 2017

superm1 left a comment

superm1 Nov 3, 2017

hughsie Nov 4, 2017

superm1 Nov 3, 2017

hughsie Nov 4, 2017

superm1 Nov 3, 2017

hughsie Nov 4, 2017

superm1 Nov 3, 2017

hughsie Nov 4, 2017

hughsie Nov 4, 2017

superm1 Nov 3, 2017

superm1 Nov 3, 2017

hughsie Nov 4, 2017

superm1 Nov 3, 2017

hughsie Nov 4, 2017

superm1 Nov 3, 2017

hughsie Nov 4, 2017

superm1 Nov 6, 2017

superm1 Nov 3, 2017

hughsie Nov 4, 2017

superm1 Nov 3, 2017

hughsie Nov 4, 2017

superm1 Nov 4, 2017

hughsie Nov 4, 2017

superm1 Nov 6, 2017

hughsie commented Nov 4, 2017

superm1 commented Nov 4, 2017

hughsie commented Nov 5, 2017

superm1 commented Nov 6, 2017

superm1 Aug 27, 2018

hughsie Aug 27, 2018

superm1 Aug 27, 2018

flashrom: Add a plugin for updating using the flashrom command line tool #295

flashrom: Add a plugin for updating using the flashrom command line tool #295

Conversation

hughsie commented Nov 3, 2017

superm1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hughsie commented Nov 4, 2017

superm1 commented Nov 4, 2017

hughsie commented Nov 5, 2017

superm1 commented Nov 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment