[OpenWrt-Devel,procd] system: reject sysupgrade of broken firmware images

From: Rafał Miłecki <rafal@milecki.pl>

This uses recently added "validate_firmware_image" to validate passed
firmware. If it happens to be invalid and marked as impossible to force
then sysupgrade simply exits with an error.

This change is needed to avoid bricking devices with some totally broken
images.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
This patch depends on the:
[PATCH procd] system: add "validate_firmware_image" ubus method
---
 system.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Hi,

> [snip]
> +	blobmsg_parse(validation_policy, __VALIDATION_MAX, validation, blob_data(b.head), blob_len(b.head));
> +
> +	valid = validation[VALIDATION_VALID] && blobmsg_get_bool(validation[VALIDATION_VALID]);
> +	forceable = validation[VALIDATION_FORCEABLE] && blobmsg_get_bool(validation[VALIDATION_FORCEABLE]);
> +
> +	if (!valid && !forceable) {
> +		fprintf(stderr, "Firmware image is broken and cannot be installed\n");
> +		return UBUS_STATUS_UNKNOWN_ERROR;

Maybe UBUS_STATUS_NOT_SUPPORTED could make sense here.

> +	}
> +
>  	sysupgrade_exec_upgraded(blobmsg_get_string(tb[SYSUPGRADE_PREFIX]),
>  				 blobmsg_get_string(tb[SYSUPGRADE_PATH]),
>  				 tb[SYSUPGRADE_COMMAND] ? blobmsg_get_string(tb[SYSUPGRADE_COMMAND]) : NULL,
>

What's the point of "force" if it doesn't force? Are we going to
add a second -F to "really force" ? Or is it going to be "oh, -F
failed for some lame reason, so I'll use mtd write, and still
complain anyway"

Cheers,
Karl P

Rafał Miłecki  <zajec5@gmail.com> wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> This uses recently added "validate_firmware_image" to validate
> passed firmware. If it happens to be invalid and marked as
> impossible to force then sysupgrade simply exits with an error.
> 
> This change is needed to avoid bricking devices with some
> totally broken images.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> This patch depends on the:
> [PATCH procd] system: add "validate_firmware_image" ubus method
> ---
>  system.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/system.c b/system.c
> index 35d5a23..7f49758 100644
> --- a/system.c
> +++ b/system.c
> @@ -507,7 +507,18 @@ static int sysupgrade(struct ubus_context *ctx, struct ubus_object *obj,
>  		      struct ubus_request_data *req, const char *method,
>  		      struct blob_attr *msg)
>  {
> +	enum {
> +		VALIDATION_VALID,
> +		VALIDATION_FORCEABLE,
> +		__VALIDATION_MAX
> +	};
> +	static const struct blobmsg_policy validation_policy[__VALIDATION_MAX] = {
> +		[VALIDATION_VALID] = { .name = "valid", .type = BLOBMSG_TYPE_BOOL },
> +		[VALIDATION_FORCEABLE] = { .name = "forceable", .type = BLOBMSG_TYPE_BOOL },
> +	};
> +	struct blob_attr *validation[__VALIDATION_MAX];
>  	struct blob_attr *tb[__SYSUPGRADE_MAX];
> +	bool valid, forceable;
>  
>  	if (!msg)
>  		return UBUS_STATUS_INVALID_ARGUMENT;
> @@ -516,6 +527,19 @@ static int sysupgrade(struct ubus_context *ctx, struct ubus_object *obj,
>  	if (!tb[SYSUPGRADE_PATH] || !tb[SYSUPGRADE_PREFIX])
>  		return UBUS_STATUS_INVALID_ARGUMENT;
>  
> +	if (validate_firmware_image_call(blobmsg_get_string(tb[SYSUPGRADE_PATH])))
> +		return UBUS_STATUS_UNKNOWN_ERROR;
> +
> +	blobmsg_parse(validation_policy, __VALIDATION_MAX, validation, blob_data(b.head), blob_len(b.head));
> +
> +	valid = validation[VALIDATION_VALID] && blobmsg_get_bool(validation[VALIDATION_VALID]);
> +	forceable = validation[VALIDATION_FORCEABLE] && blobmsg_get_bool(validation[VALIDATION_FORCEABLE]);
> +
> +	if (!valid && !forceable) {
> +		fprintf(stderr, "Firmware image is broken and cannot be installed\n");
> +		return UBUS_STATUS_UNKNOWN_ERROR;
> +	}
> +
>  	sysupgrade_exec_upgraded(blobmsg_get_string(tb[SYSUPGRADE_PREFIX]),
>  				 blobmsg_get_string(tb[SYSUPGRADE_PATH]),
>  				 tb[SYSUPGRADE_COMMAND] ? blobmsg_get_string(tb[SYSUPGRADE_COMMAND]) : NULL,
> -- 
> 2.21.0
> 
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel

This needs to be handled very carefully, not to break
actual usage of -F.
I had to use -F couple of times, usually when downgrading
installed firmware, but with change of name over time.
Typical example: Change of image name for the zbt-we826.
Never any problem with usage of -F, though.

But I had several problems with non-completion of
valid sysupgrade, which even left the system in inconsistent state,
as the "-f keep.tar.gz" was applied, but not the new image itself.
Or (silently ?) no sysupgrade done, because of mounted block device
or active swap file on block device, or active wifi (?).
Which was a PITA on remote installations.

Question: How are sysupgrade-errors reported/to be handled, as usually also a failed sysupgrade
triggered a reboot ?
documentation very unclear here, as it looks like, behaviour in case of errro changed during versions of openwrt.

Best would be "atomic sysupgrade", with standard error-code (+1) on exit instead of expected reboot after success.

I appreciate the new work on sysupgrade, but I am a bit afraid of regressions.



Am 01.09.2019 um 01:14 schrieb Karl Palsson:
> 
> What's the point of "force" if it doesn't force? Are we going to
> add a second -F to "really force" ? Or is it going to be "oh, -F
> failed for some lame reason, so I'll use mtd write, and still
> complain anyway"
> 
> Cheers,
> Karl P
> 
> Rafał Miłecki  <zajec5@gmail.com> wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> This uses recently added "validate_firmware_image" to validate
>> passed firmware. If it happens to be invalid and marked as
>> impossible to force then sysupgrade simply exits with an error.
>>
>> This change is needed to avoid bricking devices with some
>> totally broken images.
>>
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
>> This patch depends on the:
>> [PATCH procd] system: add "validate_firmware_image" ubus method
>> ---
>>   system.c | 24 ++++++++++++++++++++++++
>>   1 file changed, 24 insertions(+)
>>
>> diff --git a/system.c b/system.c
>> index 35d5a23..7f49758 100644
>> --- a/system.c
>> +++ b/system.c
>> @@ -507,7 +507,18 @@ static int sysupgrade(struct ubus_context *ctx, struct ubus_object *obj,
>>   		      struct ubus_request_data *req, const char *method,
>>   		      struct blob_attr *msg)
>>   {
>> +	enum {
>> +		VALIDATION_VALID,
>> +		VALIDATION_FORCEABLE,
>> +		__VALIDATION_MAX
>> +	};
>> +	static const struct blobmsg_policy validation_policy[__VALIDATION_MAX] = {
>> +		[VALIDATION_VALID] = { .name = "valid", .type = BLOBMSG_TYPE_BOOL },
>> +		[VALIDATION_FORCEABLE] = { .name = "forceable", .type = BLOBMSG_TYPE_BOOL },
>> +	};
>> +	struct blob_attr *validation[__VALIDATION_MAX];
>>   	struct blob_attr *tb[__SYSUPGRADE_MAX];
>> +	bool valid, forceable;
>>   
>>   	if (!msg)
>>   		return UBUS_STATUS_INVALID_ARGUMENT;
>> @@ -516,6 +527,19 @@ static int sysupgrade(struct ubus_context *ctx, struct ubus_object *obj,
>>   	if (!tb[SYSUPGRADE_PATH] || !tb[SYSUPGRADE_PREFIX])
>>   		return UBUS_STATUS_INVALID_ARGUMENT;
>>   
>> +	if (validate_firmware_image_call(blobmsg_get_string(tb[SYSUPGRADE_PATH])))
>> +		return UBUS_STATUS_UNKNOWN_ERROR;
>> +
>> +	blobmsg_parse(validation_policy, __VALIDATION_MAX, validation, blob_data(b.head), blob_len(b.head));
>> +
>> +	valid = validation[VALIDATION_VALID] && blobmsg_get_bool(validation[VALIDATION_VALID]);
>> +	forceable = validation[VALIDATION_FORCEABLE] && blobmsg_get_bool(validation[VALIDATION_FORCEABLE]);
>> +
>> +	if (!valid && !forceable) {
>> +		fprintf(stderr, "Firmware image is broken and cannot be installed\n");
>> +		return UBUS_STATUS_UNKNOWN_ERROR;
>> +	}
>> +
>>   	sysupgrade_exec_upgraded(blobmsg_get_string(tb[SYSUPGRADE_PREFIX]),
>>   				 blobmsg_get_string(tb[SYSUPGRADE_PATH]),
>>   				 tb[SYSUPGRADE_COMMAND] ? blobmsg_get_string(tb[SYSUPGRADE_COMMAND]) : NULL,
>> -- 
>> 2.21.0
>>
>>
>> _______________________________________________
>> openwrt-devel mailing list
>> openwrt-devel@lists.openwrt.org
>> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
>>
>> _______________________________________________
>> openwrt-devel mailing list
>> openwrt-devel@lists.openwrt.org
>> https://lists.openwrt.org/mailman/listinfo/openwrt-devel

On 2019-09-01 00:14, Karl Palsson wrote:
> What's the point of "force" if it doesn't force? Are we going to
> add a second -F to "really force" ? Or is it going to be "oh, -F
> failed for some lame reason, so I'll use mtd write, and still
> complain anyway"

Firmware can be marked as:
1) Invalid
2) Broken

Force option is to bypass invalid check only.

Invalid fw state may mean broken checksum or no device model match.
Broken fw state should mean unrecognized fw image format.

I'm not saying every target has to use that as it surely won't fit all
of them nicely. I can say it's going to fit Broadcom platform very well
though.

On Sun, 1 Sep 2019 at 06:13, Reiner Karlsberg <karlsberg@softart-ge.com> wrote:
> This needs to be handled very carefully, not to break
> actual usage of -F.
> I had to use -F couple of times, usually when downgrading
> installed firmware, but with change of name over time.
> Typical example: Change of image name for the zbt-we826.
> Never any problem with usage of -F, though.
>
> But I had several problems with non-completion of
> valid sysupgrade, which even left the system in inconsistent state,
> as the "-f keep.tar.gz" was applied, but not the new image itself.
> Or (silently ?) no sysupgrade done, because of mounted block device
> or active swap file on block device, or active wifi (?).
> Which was a PITA on remote installations.
>
> Question: How are sysupgrade-errors reported/to be handled, as usually also a failed sysupgrade
> triggered a reboot ?
> documentation very unclear here, as it looks like, behaviour in case of errro changed during versions of openwrt.
>
> Best would be "atomic sysupgrade", with standard error-code (+1) on exit instead of expected reboot after success.
>
> I appreciate the new work on sysupgrade, but I am a bit afraid of regressions.

No behavior will change until you explicitly modify your target's
/lib/upgrade/platform.sh to start calling notify_firmware_broken() for
whatever reason (e.g. unrecognized firmware image format).

I'm planning to implement more verbose sysupgrade results later. I was
thinking about ubus method replying with a JSON containing error code
and message. I should prepare some patch for that in a next week or
two.

Please use "Reply to all", I almost missed your reply.

Hi Rafał,

On Sun, Sep 1, 2019 at 5:09 AM Rafał Miłecki <zajec5@gmail.com> wrote:
> On Sun, 1 Sep 2019 at 06:13, Reiner Karlsberg <karlsberg@softart-ge.com>
wrote:
> > This needs to be handled very carefully, not to break
> > actual usage of -F.
> > I had to use -F couple of times, usually when downgrading
> > installed firmware, but with change of name over time.
> > Typical example: Change of image name for the zbt-we826.
> > Never any problem with usage of -F, though.
> >
> > But I had several problems with non-completion of
> > valid sysupgrade, which even left the system in inconsistent state,
> > as the "-f keep.tar.gz" was applied, but not the new image itself.
> > Or (silently ?) no sysupgrade done, because of mounted block device
> > or active swap file on block device, or active wifi (?).
> > Which was a PITA on remote installations.
> >
> > Question: How are sysupgrade-errors reported/to be handled, as usually
also a failed sysupgrade
> > triggered a reboot ?
> > documentation very unclear here, as it looks like, behaviour in case of
errro changed during versions of openwrt.
> >
> > Best would be "atomic sysupgrade", with standard error-code (+1) on
exit instead of expected reboot after success.
> >
> > I appreciate the new work on sysupgrade, but I am a bit afraid of
regressions.
>
> No behavior will change until you explicitly modify your target's
> /lib/upgrade/platform.sh to start calling notify_firmware_broken() for
> whatever reason (e.g. unrecognized firmware image format).
>
> I'm planning to implement more verbose sysupgrade results later. I was
> thinking about ubus method replying with a JSON containing error code
> and message. I should prepare some patch for that in a next week or
> two.

Since you're improving sysupgrade to reject some images, I'm throwing an
idea I had some time ago (no code, sorry).

I would be great if there is support for certain sysupgrade images to
require a settings reset (without preserving them).
The idea is to support some use cases were preserving the settings might
leave the device in a sofbrick / misconfigured state. So a reset to
defaults is mandatory, like the recent situation when migrating a
configuration from swconfig to DSA.
Sure, it could be done by migrations scripts, but they might not be 100%
reliable (cover all possible variations).

On the implementation side, it could be done using image metadata to store
an integer with the minimum version required, which could be used by
sysupgrade to compare the locally stored value and check if a reset is
mandatory or not.
(this is just one possible implementation)

Of course, an implementation would not be useful for the current issue of
swconfig->DSA, but it might be useful in the future (who knows what might
break).

Regards,
Luis Araneda.

On Tue, 3 Sep 2019 at 18:57, Luis Araneda <luaraneda@gmail.com> wrote:
> On Sun, Sep 1, 2019 at 5:09 AM Rafał Miłecki <zajec5@gmail.com> wrote:
> > On Sun, 1 Sep 2019 at 06:13, Reiner Karlsberg <karlsberg@softart-ge.com> wrote:
> > > This needs to be handled very carefully, not to break
> > > actual usage of -F.
> > > I had to use -F couple of times, usually when downgrading
> > > installed firmware, but with change of name over time.
> > > Typical example: Change of image name for the zbt-we826.
> > > Never any problem with usage of -F, though.
> > >
> > > But I had several problems with non-completion of
> > > valid sysupgrade, which even left the system in inconsistent state,
> > > as the "-f keep.tar.gz" was applied, but not the new image itself.
> > > Or (silently ?) no sysupgrade done, because of mounted block device
> > > or active swap file on block device, or active wifi (?).
> > > Which was a PITA on remote installations.
> > >
> > > Question: How are sysupgrade-errors reported/to be handled, as usually also a failed sysupgrade
> > > triggered a reboot ?
> > > documentation very unclear here, as it looks like, behaviour in case of errro changed during versions of openwrt.
> > >
> > > Best would be "atomic sysupgrade", with standard error-code (+1) on exit instead of expected reboot after success.
> > >
> > > I appreciate the new work on sysupgrade, but I am a bit afraid of regressions.
> >
> > No behavior will change until you explicitly modify your target's
> > /lib/upgrade/platform.sh to start calling notify_firmware_broken() for
> > whatever reason (e.g. unrecognized firmware image format).
> >
> > I'm planning to implement more verbose sysupgrade results later. I was
> > thinking about ubus method replying with a JSON containing error code
> > and message. I should prepare some patch for that in a next week or
> > two.
>
> Since you're improving sysupgrade to reject some images, I'm throwing an idea I had some time ago (no code, sorry).
>
> I would be great if there is support for certain sysupgrade images to require a settings reset (without preserving them).
> The idea is to support some use cases were preserving the settings might leave the device in a sofbrick / misconfigured state. So a reset to defaults is mandatory, like the recent situation when migrating a configuration from swconfig to DSA.
> Sure, it could be done by migrations scripts, but they might not be 100% reliable (cover all possible variations).
>
> On the implementation side, it could be done using image metadata to store an integer with the minimum version required, which could be used by sysupgrade to compare the locally stored value and check if a reset is mandatory or not.
> (this is just one possible implementation)
>
> Of course, an implementation would not be useful for the current issue of swconfig->DSA, but it might be useful in the future (who knows what might break).

I see some use-cases for that. I'll implement that.