Skip to content

Commit

Permalink
[K32W] Fix apply action corner case in OTATlvProcessor interface (#33214
Browse files Browse the repository at this point in the history 
)

* [k32w] Add mShouldNotApply flag in OTATlvProcessor interface

OTATlvProcessor::ApplyAction now has a default implementation,
but derived classes are still able to overwrite this.

The flag is used by the default ApplyAction implementation.
If something goes wrong during ExitAction of the TLV processor,
then mShouldNotApply should be set to true and the image processor
should abort. In this case, the BDX transfer was already finished
and calling CancelImageUpdate will not abort the transfer, hence
the device will reboot even though it should not have. If ApplyAction
fails during HandleApply, then the process will be aborted.

Signed-off-by: marius-alex-tache <[email protected]>

* [k32w0] Use mShouldNotApply flag in ExitAction

During ExitAction, set mShouldNotApply to true if an error occurs.
This ensures that the OTA will be aborted and the device does not
reboot.

Also remove the ApplyAction override, since the default implementation
is enough.

Signed-off-by: marius-alex-tache <[email protected]>

* [k32w1] Use mShouldNotApply during ExitAction

Signed-off-by: marius-alex-tache <[email protected]>

* [k32w] Update OTA error naming

All OTA errors should be prefixed with CHIP_ERROR.

Signed-off-by: marius-alex-tache <[email protected]>

* [k32w] Replace boolean mShouldNotApply with an enum class

Signed-off-by: marius-alex-tache <[email protected]>

---------

Signed-off-by: marius-alex-tache <[email protected]>
  • Loading branch information
@marius-alex-tache @pull
marius-alex-tache authored and pull[bot] committed Oct 15, 2024
1 parent 8e77ba2 commit 46f8df2
Show file tree
Hide file tree
Showing 6 changed files with 79 additions and 56 deletions.
8 changes: 4 additions & 4 deletions src/platform/nxp/k32w/common/OTAImageProcessorImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ CHIP_ERROR OTAImageProcessorImpl::ProcessPayload(ByteSpan & block)
}

status = mCurrentProcessor->Process(block);
if (status == CHIP_OTA_CHANGE_PROCESSOR)
if (status == CHIP_ERROR_OTA_CHANGE_PROCESSOR)
{
mAccumulator.Clear();
mAccumulator.Init(sizeof(OTATlvHeader));
Expand Down Expand Up @@ -180,7 +180,7 @@ CHIP_ERROR OTAImageProcessorImpl::SelectProcessor(ByteSpan & block)
if (pair == mProcessorMap.end())
{
ChipLogError(SoftwareUpdate, "There is no registered processor for tag: %" PRIu32, header.tag);
return CHIP_OTA_PROCESSOR_NOT_REGISTERED;
return CHIP_ERROR_OTA_PROCESSOR_NOT_REGISTERED;
}

ChipLogDetail(SoftwareUpdate, "Selected processor with tag: %ld", pair->first);
Expand All @@ -197,7 +197,7 @@ CHIP_ERROR OTAImageProcessorImpl::RegisterProcessor(uint32_t tag, OTATlvProcesso
if (pair != mProcessorMap.end())
{
ChipLogError(SoftwareUpdate, "A processor for tag %" PRIu32 " is already registered.", tag);
return CHIP_OTA_PROCESSOR_ALREADY_REGISTERED;
return CHIP_ERROR_OTA_PROCESSOR_ALREADY_REGISTERED;
}

mProcessorMap.insert({ tag, processor });
Expand Down Expand Up @@ -249,7 +249,7 @@ void OTAImageProcessorImpl::HandleStatus(CHIP_ERROR status)
mParams.downloadedBytes += mBlock.size();
FetchNextData(0);
}
else if (status == CHIP_OTA_FETCH_ALREADY_SCHEDULED)
else if (status == CHIP_ERROR_OTA_FETCH_ALREADY_SCHEDULED)
{
mParams.downloadedBytes += mBlock.size();
}
Expand Down
11 changes: 9 additions & 2 deletions src/platform/nxp/k32w/common/OTATlvProcessor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,12 @@ namespace chip {
#if OTA_ENCRYPTION_ENABLE
constexpr uint8_t au8Iv[] = { 0x00, 0x00, 0x00, 0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18, 0x00, 0x00, 0x00, 0x00 };
#endif

CHIP_ERROR OTATlvProcessor::ApplyAction()
{
return mApplyState == ApplyState::kApply ? CHIP_NO_ERROR : CHIP_ERROR_OTA_PROCESSOR_DO_NOT_APPLY;
}

CHIP_ERROR OTATlvProcessor::Process(ByteSpan & block)
{
CHIP_ERROR status = CHIP_NO_ERROR;
Expand All @@ -50,7 +56,7 @@ CHIP_ERROR OTATlvProcessor::Process(ByteSpan & block)
// If current block was processed fully and the block still contains data, it
// means that the block contains another TLV's data and the current processor
// should be changed by OTAImageProcessorImpl.
return CHIP_OTA_CHANGE_PROCESSOR;
return CHIP_ERROR_OTA_CHANGE_PROCESSOR;
}
}
}
Expand All @@ -63,14 +69,15 @@ void OTATlvProcessor::ClearInternal()
mLength = 0;
mProcessedLength = 0;
mWasSelected = false;
mApplyState = ApplyState::kApply;
#if OTA_ENCRYPTION_ENABLE
mIVOffset = 0;
#endif
}

bool OTATlvProcessor::IsError(CHIP_ERROR & status)
{
return status != CHIP_NO_ERROR && status != CHIP_ERROR_BUFFER_TOO_SMALL && status != CHIP_OTA_FETCH_ALREADY_SCHEDULED;
return status != CHIP_NO_ERROR && status != CHIP_ERROR_BUFFER_TOO_SMALL && status != CHIP_ERROR_OTA_FETCH_ALREADY_SCHEDULED;
}

void OTADataAccumulator::Init(uint32_t threshold)
Expand Down
72 changes: 46 additions & 26 deletions src/platform/nxp/k32w/common/OTATlvProcessor.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,20 +27,20 @@ namespace chip {
#define CHIP_ERROR_TLV_PROCESSOR(e) \
ChipError(ChipError::Range::kLastRange, ((uint8_t) ChipError::Range::kLastRange << 3) | e, __FILE__, __LINE__)

#define CHIP_OTA_TLV_CONTINUE_PROCESSING CHIP_ERROR_TLV_PROCESSOR(0x01)
#define CHIP_OTA_CHANGE_PROCESSOR CHIP_ERROR_TLV_PROCESSOR(0x02)
#define CHIP_OTA_PROCESSOR_NOT_REGISTERED CHIP_ERROR_TLV_PROCESSOR(0x03)
#define CHIP_OTA_PROCESSOR_ALREADY_REGISTERED CHIP_ERROR_TLV_PROCESSOR(0x04)
#define CHIP_OTA_PROCESSOR_CLIENT_INIT CHIP_ERROR_TLV_PROCESSOR(0x05)
#define CHIP_OTA_PROCESSOR_MAKE_ROOM CHIP_ERROR_TLV_PROCESSOR(0x06)
#define CHIP_OTA_PROCESSOR_PUSH_CHUNK CHIP_ERROR_TLV_PROCESSOR(0x07)
#define CHIP_OTA_PROCESSOR_IMG_AUTH CHIP_ERROR_TLV_PROCESSOR(0x08)
#define CHIP_OTA_FETCH_ALREADY_SCHEDULED CHIP_ERROR_TLV_PROCESSOR(0x09)
#define CHIP_OTA_PROCESSOR_IMG_COMMIT CHIP_ERROR_TLV_PROCESSOR(0x0A)
#define CHIP_OTA_PROCESSOR_CB_NOT_REGISTERED CHIP_ERROR_TLV_PROCESSOR(0x0B)
#define CHIP_OTA_PROCESSOR_EEPROM_OFFSET CHIP_ERROR_TLV_PROCESSOR(0x0C)
#define CHIP_OTA_PROCESSOR_EXTERNAL_STORAGE CHIP_ERROR_TLV_PROCESSOR(0x0D)
#define CHIP_OTA_PROCESSOR_START_IMAGE CHIP_ERROR_TLV_PROCESSOR(0x0E)
#define CHIP_ERROR_OTA_CHANGE_PROCESSOR CHIP_ERROR_TLV_PROCESSOR(0x02)
#define CHIP_ERROR_OTA_PROCESSOR_NOT_REGISTERED CHIP_ERROR_TLV_PROCESSOR(0x03)
#define CHIP_ERROR_OTA_PROCESSOR_ALREADY_REGISTERED CHIP_ERROR_TLV_PROCESSOR(0x04)
#define CHIP_ERROR_OTA_PROCESSOR_CLIENT_INIT CHIP_ERROR_TLV_PROCESSOR(0x05)
#define CHIP_ERROR_OTA_PROCESSOR_MAKE_ROOM CHIP_ERROR_TLV_PROCESSOR(0x06)
#define CHIP_ERROR_OTA_PROCESSOR_PUSH_CHUNK CHIP_ERROR_TLV_PROCESSOR(0x07)
#define CHIP_ERROR_OTA_PROCESSOR_IMG_AUTH CHIP_ERROR_TLV_PROCESSOR(0x08)
#define CHIP_ERROR_OTA_FETCH_ALREADY_SCHEDULED CHIP_ERROR_TLV_PROCESSOR(0x09)
#define CHIP_ERROR_OTA_PROCESSOR_IMG_COMMIT CHIP_ERROR_TLV_PROCESSOR(0x0A)
#define CHIP_ERROR_OTA_PROCESSOR_CB_NOT_REGISTERED CHIP_ERROR_TLV_PROCESSOR(0x0B)
#define CHIP_ERROR_OTA_PROCESSOR_EEPROM_OFFSET CHIP_ERROR_TLV_PROCESSOR(0x0C)
#define CHIP_ERROR_OTA_PROCESSOR_EXTERNAL_STORAGE CHIP_ERROR_TLV_PROCESSOR(0x0D)
#define CHIP_ERROR_OTA_PROCESSOR_START_IMAGE CHIP_ERROR_TLV_PROCESSOR(0x0E)
#define CHIP_ERROR_OTA_PROCESSOR_DO_NOT_APPLY CHIP_ERROR_TLV_PROCESSOR(0x0F)

// Descriptor constants
constexpr size_t kVersionStringSize = 64;
Expand Down Expand Up @@ -77,13 +77,19 @@ struct OTATlvHeader
class OTATlvProcessor
{
public:
enum class ApplyState : uint8_t
{
kApply = 0,
kDoNotApply
};

virtual ~OTATlvProcessor() {}

virtual CHIP_ERROR Init() = 0;
virtual CHIP_ERROR Clear() = 0;
virtual CHIP_ERROR ApplyAction() = 0;
virtual CHIP_ERROR AbortAction() = 0;
virtual CHIP_ERROR ExitAction() { return CHIP_NO_ERROR; }
virtual CHIP_ERROR ApplyAction();

CHIP_ERROR Process(ByteSpan & block);
void RegisterDescriptorCallback(ProcessDescriptor callback) { mCallbackProcessDescriptor = callback; }
Expand All @@ -102,21 +108,21 @@ class OTATlvProcessor
* If more image chunks are needed, CHIP_ERROR_BUFFER_TOO_SMALL error is returned.
* Other error codes indicate that an error occurred during processing. Fetching
* next data is scheduled automatically by OTAImageProcessorImpl if the return value
* is neither an error code, nor CHIP_OTA_FETCH_ALREADY_SCHEDULED (which implies the
* is neither an error code, nor CHIP_ERROR_OTA_FETCH_ALREADY_SCHEDULED (which implies the
* scheduling is done inside ProcessInternal or will be done in the future, through a
* callback).
*
* @param block Byte span containing a subsequent Matter OTA image chunk. When the method
* returns CHIP_NO_ERROR, the byte span is used to return a remaining part
* of the chunk, not used by current TLV processor.
*
* @retval CHIP_NO_ERROR Block was processed successfully.
* @retval CHIP_ERROR_BUFFER_TOO_SMALL Provided buffers are insufficient to decode some
* metadata (e.g. a descriptor).
* @retval CHIP_OTA_FETCH_ALREADY_SCHEDULED Should be returned if ProcessInternal schedules
* fetching next data (e.g. through a callback).
* @retval Error code Something went wrong. Current OTA process will be
* canceled.
* @retval CHIP_NO_ERROR Block was processed successfully.
* @retval CHIP_ERROR_BUFFER_TOO_SMALL Provided buffers are insufficient to decode some
* metadata (e.g. a descriptor).
* @retval CHIP_ERROR_OTA_FETCH_ALREADY_SCHEDULED Should be returned if ProcessInternal schedules
* fetching next data (e.g. through a callback).
* @retval Error code Something went wrong. Current OTA process will be
* canceled.
*/
virtual CHIP_ERROR ProcessInternal(ByteSpan & block) = 0;

Expand All @@ -130,9 +136,23 @@ class OTATlvProcessor
/* Expected byte size of the OTAEncryptionKeyLength */
static constexpr size_t kOTAEncryptionKeyLength = 16;
#endif
uint32_t mLength = 0;
uint32_t mProcessedLength = 0;
bool mWasSelected = false;
uint32_t mLength = 0;
uint32_t mProcessedLength = 0;
bool mWasSelected = false;

/**
* @brief A flag to account for corner cases during OTA apply
*
* Used by the default ApplyAction implementation.
*
* If something goes wrong during ExitAction of the TLV processor,
* then mApplyState should be set to kDoNotApply and the image processor
* should abort. In this case, the BDX transfer was already finished
* and calling CancelImageUpdate will not abort the transfer, hence
* the device will reboot even though it should not have. If ApplyAction
* fails during HandleApply, then the process will be aborted.
*/
ApplyState mApplyState = ApplyState::kApply;
ProcessDescriptor mCallbackProcessDescriptor = nullptr;
};

Expand Down
26 changes: 11 additions & 15 deletions src/platform/nxp/k32w/k32w0/OTAFirmwareProcessor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,21 +28,21 @@ namespace chip {

CHIP_ERROR OTAFirmwareProcessor::Init()
{
ReturnErrorCodeIf(mCallbackProcessDescriptor == nullptr, CHIP_OTA_PROCESSOR_CB_NOT_REGISTERED);
ReturnErrorCodeIf(mCallbackProcessDescriptor == nullptr, CHIP_ERROR_OTA_PROCESSOR_CB_NOT_REGISTERED);
mAccumulator.Init(sizeof(Descriptor));
#if OTA_ENCRYPTION_ENABLE
mUnalignmentNum = 0;
#endif
ReturnErrorCodeIf(gOtaSuccess_c != OTA_ClientInit(), CHIP_OTA_PROCESSOR_CLIENT_INIT);
ReturnErrorCodeIf(gOtaSuccess_c != OTA_ClientInit(), CHIP_ERROR_OTA_PROCESSOR_CLIENT_INIT);

auto offset = OTA_GetCurrentEepromAddressOffset();
if (offset != 0)
{
offset += 1;
}

ReturnErrorCodeIf(OTA_UTILS_IMAGE_INVALID_ADDR == OTA_SetStartEepromOffset(offset), CHIP_OTA_PROCESSOR_EEPROM_OFFSET);
ReturnErrorCodeIf(gOtaSuccess_c != OTA_StartImage(mLength - sizeof(Descriptor)), CHIP_OTA_PROCESSOR_START_IMAGE);
ReturnErrorCodeIf(OTA_UTILS_IMAGE_INVALID_ADDR == OTA_SetStartEepromOffset(offset), CHIP_ERROR_OTA_PROCESSOR_EEPROM_OFFSET);
ReturnErrorCodeIf(gOtaSuccess_c != OTA_StartImage(mLength - sizeof(Descriptor)), CHIP_ERROR_OTA_PROCESSOR_START_IMAGE);

return CHIP_NO_ERROR;
}
Expand Down Expand Up @@ -95,7 +95,7 @@ CHIP_ERROR OTAFirmwareProcessor::ProcessInternal(ByteSpan & block)
if (gOtaSuccess_c != status)
{
ChipLogError(SoftwareUpdate, "Failed to make room for next block. Status: %d", status);
return CHIP_OTA_PROCESSOR_MAKE_ROOM;
return CHIP_ERROR_OTA_PROCESSOR_MAKE_ROOM;
}
#if OTA_ENCRYPTION_ENABLE
status = OTA_PushImageChunk((uint8_t *) mBlock.data(), (uint16_t) mBlock.size(), NULL, NULL);
Expand All @@ -105,10 +105,10 @@ CHIP_ERROR OTAFirmwareProcessor::ProcessInternal(ByteSpan & block)
if (gOtaSuccess_c != status)
{
ChipLogError(SoftwareUpdate, "Failed to write image block. Status: %d", status);
return CHIP_OTA_PROCESSOR_PUSH_CHUNK;
return CHIP_ERROR_OTA_PROCESSOR_PUSH_CHUNK;
}

return CHIP_OTA_FETCH_ALREADY_SCHEDULED;
return CHIP_ERROR_OTA_FETCH_ALREADY_SCHEDULED;
}

CHIP_ERROR OTAFirmwareProcessor::ProcessDescriptor(ByteSpan & block)
Expand All @@ -122,12 +122,6 @@ CHIP_ERROR OTAFirmwareProcessor::ProcessDescriptor(ByteSpan & block)
return CHIP_NO_ERROR;
}

CHIP_ERROR OTAFirmwareProcessor::ApplyAction()
{

return CHIP_NO_ERROR;
}

CHIP_ERROR OTAFirmwareProcessor::AbortAction()
{
OTA_CancelImage();
Expand All @@ -144,13 +138,15 @@ CHIP_ERROR OTAFirmwareProcessor::ExitAction()
if (OTA_CommitImage(NULL) != gOtaSuccess_c)
{
ChipLogError(SoftwareUpdate, "Failed to commit firmware image.");
return CHIP_OTA_PROCESSOR_IMG_COMMIT;
mApplyState = ApplyState::kDoNotApply;
return CHIP_ERROR_OTA_PROCESSOR_IMG_COMMIT;
}

if (OTA_ImageAuthenticate() != gOtaImageAuthPass_c)
{
ChipLogError(SoftwareUpdate, "Failed to authenticate firmware image.");
return CHIP_OTA_PROCESSOR_IMG_AUTH;
mApplyState = ApplyState::kDoNotApply;
return CHIP_ERROR_OTA_PROCESSOR_IMG_AUTH;
}

OTA_AddNewImageFlag();
Expand Down
1 change: 0 additions & 1 deletion src/platform/nxp/k32w/k32w0/OTAFirmwareProcessor.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ class OTAFirmwareProcessor : public OTATlvProcessor

CHIP_ERROR Init() override;
CHIP_ERROR Clear() override;
CHIP_ERROR ApplyAction() override;
CHIP_ERROR AbortAction() override;
CHIP_ERROR ExitAction() override;

Expand Down
17 changes: 9 additions & 8 deletions src/platform/nxp/k32w/k32w1/OTAFirmwareProcessor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,16 +26,16 @@ namespace chip {

CHIP_ERROR OTAFirmwareProcessor::Init()
{
ReturnErrorCodeIf(mCallbackProcessDescriptor == nullptr, CHIP_OTA_PROCESSOR_CB_NOT_REGISTERED);
ReturnErrorCodeIf(mCallbackProcessDescriptor == nullptr, CHIP_ERROR_OTA_PROCESSOR_CB_NOT_REGISTERED);
mAccumulator.Init(sizeof(Descriptor));

ReturnErrorCodeIf(gOtaSuccess_c != OTA_SelectExternalStoragePartition(), CHIP_OTA_PROCESSOR_EXTERNAL_STORAGE);
ReturnErrorCodeIf(gOtaSuccess_c != OTA_SelectExternalStoragePartition(), CHIP_ERROR_OTA_PROCESSOR_EXTERNAL_STORAGE);

otaResult_t ota_status;
ota_status = OTA_ServiceInit(&mPostedOperationsStorage[0], NB_PENDING_TRANSACTIONS * TRANSACTION_SZ);

ReturnErrorCodeIf(ota_status != gOtaSuccess_c, CHIP_OTA_PROCESSOR_CLIENT_INIT);
ReturnErrorCodeIf(gOtaSuccess_c != OTA_StartImage(mLength - sizeof(Descriptor)), CHIP_OTA_PROCESSOR_START_IMAGE);
ReturnErrorCodeIf(ota_status != gOtaSuccess_c, CHIP_ERROR_OTA_PROCESSOR_CLIENT_INIT);
ReturnErrorCodeIf(gOtaSuccess_c != OTA_StartImage(mLength - sizeof(Descriptor)), CHIP_ERROR_OTA_PROCESSOR_START_IMAGE);

return CHIP_NO_ERROR;
}
Expand Down Expand Up @@ -70,7 +70,7 @@ CHIP_ERROR OTAFirmwareProcessor::ProcessInternal(ByteSpan & block)
if (gOtaSuccess_c != status)
{
ChipLogError(SoftwareUpdate, "Failed to make room for next block. Status: %d", status);
return CHIP_OTA_PROCESSOR_MAKE_ROOM;
return CHIP_ERROR_OTA_PROCESSOR_MAKE_ROOM;
}
}
else
Expand All @@ -82,10 +82,10 @@ CHIP_ERROR OTAFirmwareProcessor::ProcessInternal(ByteSpan & block)
if (gOtaSuccess_c != status)
{
ChipLogError(SoftwareUpdate, "Failed to write image block. Status: %d", status);
return CHIP_OTA_PROCESSOR_PUSH_CHUNK;
return CHIP_ERROR_OTA_PROCESSOR_PUSH_CHUNK;
}

return CHIP_OTA_FETCH_ALREADY_SCHEDULED;
return CHIP_ERROR_OTA_FETCH_ALREADY_SCHEDULED;
}

CHIP_ERROR OTAFirmwareProcessor::ProcessDescriptor(ByteSpan & block)
Expand Down Expand Up @@ -117,7 +117,8 @@ CHIP_ERROR OTAFirmwareProcessor::ExitAction()
if (OTA_CommitImage(NULL) != gOtaSuccess_c)
{
ChipLogError(SoftwareUpdate, "Failed to commit firmware image.");
return CHIP_OTA_PROCESSOR_IMG_COMMIT;
mApplyState = ApplyState::kDoNotApply;
return CHIP_ERROR_OTA_PROCESSOR_IMG_COMMIT;
}

return CHIP_NO_ERROR;
Expand Down

0 comments on commit 46f8df2

Please sign in to comment.