Message ID | 20190830154607.6463-1-zajec5@gmail.com |
---|---|
State | Accepted |
Delegated to: | Rafał Miłecki |
Headers | show |
Series | [OpenWrt-Devel,procd] system: reject sysupgrade of broken firmware images | expand |
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.
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,