Skip to content

Commit 3f9bac2

Browse files
[K32W] Fix apply action corner case in OTATlvProcessor interface (#33214)
* [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 <marius.tache@nxp.com> * [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 <marius.tache@nxp.com> * [k32w1] Use mShouldNotApply during ExitAction Signed-off-by: marius-alex-tache <marius.tache@nxp.com> * [k32w] Update OTA error naming All OTA errors should be prefixed with CHIP_ERROR. Signed-off-by: marius-alex-tache <marius.tache@nxp.com> * [k32w] Replace boolean mShouldNotApply with an enum class Signed-off-by: marius-alex-tache <marius.tache@nxp.com> --------- Signed-off-by: marius-alex-tache <marius.tache@nxp.com>
1 parent ce7d400 commit 3f9bac2

6 files changed

+79
-56
lines changed

src/platform/nxp/k32w/common/OTAImageProcessorImpl.cpp

+4-4
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,7 @@ CHIP_ERROR OTAImageProcessorImpl::ProcessPayload(ByteSpan & block)
142142
}
143143

144144
status = mCurrentProcessor->Process(block);
145-
if (status == CHIP_OTA_CHANGE_PROCESSOR)
145+
if (status == CHIP_ERROR_OTA_CHANGE_PROCESSOR)
146146
{
147147
mAccumulator.Clear();
148148
mAccumulator.Init(sizeof(OTATlvHeader));
@@ -180,7 +180,7 @@ CHIP_ERROR OTAImageProcessorImpl::SelectProcessor(ByteSpan & block)
180180
if (pair == mProcessorMap.end())
181181
{
182182
ChipLogError(SoftwareUpdate, "There is no registered processor for tag: %" PRIu32, header.tag);
183-
return CHIP_OTA_PROCESSOR_NOT_REGISTERED;
183+
return CHIP_ERROR_OTA_PROCESSOR_NOT_REGISTERED;
184184
}
185185

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

203203
mProcessorMap.insert({ tag, processor });
@@ -249,7 +249,7 @@ void OTAImageProcessorImpl::HandleStatus(CHIP_ERROR status)
249249
mParams.downloadedBytes += mBlock.size();
250250
FetchNextData(0);
251251
}
252-
else if (status == CHIP_OTA_FETCH_ALREADY_SCHEDULED)
252+
else if (status == CHIP_ERROR_OTA_FETCH_ALREADY_SCHEDULED)
253253
{
254254
mParams.downloadedBytes += mBlock.size();
255255
}

src/platform/nxp/k32w/common/OTATlvProcessor.cpp

+9-2
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,12 @@ namespace chip {
3131
#if OTA_ENCRYPTION_ENABLE
3232
constexpr uint8_t au8Iv[] = { 0x00, 0x00, 0x00, 0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18, 0x00, 0x00, 0x00, 0x00 };
3333
#endif
34+
35+
CHIP_ERROR OTATlvProcessor::ApplyAction()
36+
{
37+
return mApplyState == ApplyState::kApply ? CHIP_NO_ERROR : CHIP_ERROR_OTA_PROCESSOR_DO_NOT_APPLY;
38+
}
39+
3440
CHIP_ERROR OTATlvProcessor::Process(ByteSpan & block)
3541
{
3642
CHIP_ERROR status = CHIP_NO_ERROR;
@@ -50,7 +56,7 @@ CHIP_ERROR OTATlvProcessor::Process(ByteSpan & block)
5056
// If current block was processed fully and the block still contains data, it
5157
// means that the block contains another TLV's data and the current processor
5258
// should be changed by OTAImageProcessorImpl.
53-
return CHIP_OTA_CHANGE_PROCESSOR;
59+
return CHIP_ERROR_OTA_CHANGE_PROCESSOR;
5460
}
5561
}
5662
}
@@ -63,14 +69,15 @@ void OTATlvProcessor::ClearInternal()
6369
mLength = 0;
6470
mProcessedLength = 0;
6571
mWasSelected = false;
72+
mApplyState = ApplyState::kApply;
6673
#if OTA_ENCRYPTION_ENABLE
6774
mIVOffset = 0;
6875
#endif
6976
}
7077

7178
bool OTATlvProcessor::IsError(CHIP_ERROR & status)
7279
{
73-
return status != CHIP_NO_ERROR && status != CHIP_ERROR_BUFFER_TOO_SMALL && status != CHIP_OTA_FETCH_ALREADY_SCHEDULED;
80+
return status != CHIP_NO_ERROR && status != CHIP_ERROR_BUFFER_TOO_SMALL && status != CHIP_ERROR_OTA_FETCH_ALREADY_SCHEDULED;
7481
}
7582

7683
void OTADataAccumulator::Init(uint32_t threshold)

src/platform/nxp/k32w/common/OTATlvProcessor.h

+46-26
Original file line numberDiff line numberDiff line change
@@ -27,20 +27,20 @@ namespace chip {
2727
#define CHIP_ERROR_TLV_PROCESSOR(e) \
2828
ChipError(ChipError::Range::kLastRange, ((uint8_t) ChipError::Range::kLastRange << 3) | e, __FILE__, __LINE__)
2929

30-
#define CHIP_OTA_TLV_CONTINUE_PROCESSING CHIP_ERROR_TLV_PROCESSOR(0x01)
31-
#define CHIP_OTA_CHANGE_PROCESSOR CHIP_ERROR_TLV_PROCESSOR(0x02)
32-
#define CHIP_OTA_PROCESSOR_NOT_REGISTERED CHIP_ERROR_TLV_PROCESSOR(0x03)
33-
#define CHIP_OTA_PROCESSOR_ALREADY_REGISTERED CHIP_ERROR_TLV_PROCESSOR(0x04)
34-
#define CHIP_OTA_PROCESSOR_CLIENT_INIT CHIP_ERROR_TLV_PROCESSOR(0x05)
35-
#define CHIP_OTA_PROCESSOR_MAKE_ROOM CHIP_ERROR_TLV_PROCESSOR(0x06)
36-
#define CHIP_OTA_PROCESSOR_PUSH_CHUNK CHIP_ERROR_TLV_PROCESSOR(0x07)
37-
#define CHIP_OTA_PROCESSOR_IMG_AUTH CHIP_ERROR_TLV_PROCESSOR(0x08)
38-
#define CHIP_OTA_FETCH_ALREADY_SCHEDULED CHIP_ERROR_TLV_PROCESSOR(0x09)
39-
#define CHIP_OTA_PROCESSOR_IMG_COMMIT CHIP_ERROR_TLV_PROCESSOR(0x0A)
40-
#define CHIP_OTA_PROCESSOR_CB_NOT_REGISTERED CHIP_ERROR_TLV_PROCESSOR(0x0B)
41-
#define CHIP_OTA_PROCESSOR_EEPROM_OFFSET CHIP_ERROR_TLV_PROCESSOR(0x0C)
42-
#define CHIP_OTA_PROCESSOR_EXTERNAL_STORAGE CHIP_ERROR_TLV_PROCESSOR(0x0D)
43-
#define CHIP_OTA_PROCESSOR_START_IMAGE CHIP_ERROR_TLV_PROCESSOR(0x0E)
30+
#define CHIP_ERROR_OTA_CHANGE_PROCESSOR CHIP_ERROR_TLV_PROCESSOR(0x02)
31+
#define CHIP_ERROR_OTA_PROCESSOR_NOT_REGISTERED CHIP_ERROR_TLV_PROCESSOR(0x03)
32+
#define CHIP_ERROR_OTA_PROCESSOR_ALREADY_REGISTERED CHIP_ERROR_TLV_PROCESSOR(0x04)
33+
#define CHIP_ERROR_OTA_PROCESSOR_CLIENT_INIT CHIP_ERROR_TLV_PROCESSOR(0x05)
34+
#define CHIP_ERROR_OTA_PROCESSOR_MAKE_ROOM CHIP_ERROR_TLV_PROCESSOR(0x06)
35+
#define CHIP_ERROR_OTA_PROCESSOR_PUSH_CHUNK CHIP_ERROR_TLV_PROCESSOR(0x07)
36+
#define CHIP_ERROR_OTA_PROCESSOR_IMG_AUTH CHIP_ERROR_TLV_PROCESSOR(0x08)
37+
#define CHIP_ERROR_OTA_FETCH_ALREADY_SCHEDULED CHIP_ERROR_TLV_PROCESSOR(0x09)
38+
#define CHIP_ERROR_OTA_PROCESSOR_IMG_COMMIT CHIP_ERROR_TLV_PROCESSOR(0x0A)
39+
#define CHIP_ERROR_OTA_PROCESSOR_CB_NOT_REGISTERED CHIP_ERROR_TLV_PROCESSOR(0x0B)
40+
#define CHIP_ERROR_OTA_PROCESSOR_EEPROM_OFFSET CHIP_ERROR_TLV_PROCESSOR(0x0C)
41+
#define CHIP_ERROR_OTA_PROCESSOR_EXTERNAL_STORAGE CHIP_ERROR_TLV_PROCESSOR(0x0D)
42+
#define CHIP_ERROR_OTA_PROCESSOR_START_IMAGE CHIP_ERROR_TLV_PROCESSOR(0x0E)
43+
#define CHIP_ERROR_OTA_PROCESSOR_DO_NOT_APPLY CHIP_ERROR_TLV_PROCESSOR(0x0F)
4444

4545
// Descriptor constants
4646
constexpr size_t kVersionStringSize = 64;
@@ -77,13 +77,19 @@ struct OTATlvHeader
7777
class OTATlvProcessor
7878
{
7979
public:
80+
enum class ApplyState : uint8_t
81+
{
82+
kApply = 0,
83+
kDoNotApply
84+
};
85+
8086
virtual ~OTATlvProcessor() {}
8187

8288
virtual CHIP_ERROR Init() = 0;
8389
virtual CHIP_ERROR Clear() = 0;
84-
virtual CHIP_ERROR ApplyAction() = 0;
8590
virtual CHIP_ERROR AbortAction() = 0;
8691
virtual CHIP_ERROR ExitAction() { return CHIP_NO_ERROR; }
92+
virtual CHIP_ERROR ApplyAction();
8793

8894
CHIP_ERROR Process(ByteSpan & block);
8995
void RegisterDescriptorCallback(ProcessDescriptor callback) { mCallbackProcessDescriptor = callback; }
@@ -102,21 +108,21 @@ class OTATlvProcessor
102108
* If more image chunks are needed, CHIP_ERROR_BUFFER_TOO_SMALL error is returned.
103109
* Other error codes indicate that an error occurred during processing. Fetching
104110
* next data is scheduled automatically by OTAImageProcessorImpl if the return value
105-
* is neither an error code, nor CHIP_OTA_FETCH_ALREADY_SCHEDULED (which implies the
111+
* is neither an error code, nor CHIP_ERROR_OTA_FETCH_ALREADY_SCHEDULED (which implies the
106112
* scheduling is done inside ProcessInternal or will be done in the future, through a
107113
* callback).
108114
*
109115
* @param block Byte span containing a subsequent Matter OTA image chunk. When the method
110116
* returns CHIP_NO_ERROR, the byte span is used to return a remaining part
111117
* of the chunk, not used by current TLV processor.
112118
*
113-
* @retval CHIP_NO_ERROR Block was processed successfully.
114-
* @retval CHIP_ERROR_BUFFER_TOO_SMALL Provided buffers are insufficient to decode some
115-
* metadata (e.g. a descriptor).
116-
* @retval CHIP_OTA_FETCH_ALREADY_SCHEDULED Should be returned if ProcessInternal schedules
117-
* fetching next data (e.g. through a callback).
118-
* @retval Error code Something went wrong. Current OTA process will be
119-
* canceled.
119+
* @retval CHIP_NO_ERROR Block was processed successfully.
120+
* @retval CHIP_ERROR_BUFFER_TOO_SMALL Provided buffers are insufficient to decode some
121+
* metadata (e.g. a descriptor).
122+
* @retval CHIP_ERROR_OTA_FETCH_ALREADY_SCHEDULED Should be returned if ProcessInternal schedules
123+
* fetching next data (e.g. through a callback).
124+
* @retval Error code Something went wrong. Current OTA process will be
125+
* canceled.
120126
*/
121127
virtual CHIP_ERROR ProcessInternal(ByteSpan & block) = 0;
122128

@@ -130,9 +136,23 @@ class OTATlvProcessor
130136
/* Expected byte size of the OTAEncryptionKeyLength */
131137
static constexpr size_t kOTAEncryptionKeyLength = 16;
132138
#endif
133-
uint32_t mLength = 0;
134-
uint32_t mProcessedLength = 0;
135-
bool mWasSelected = false;
139+
uint32_t mLength = 0;
140+
uint32_t mProcessedLength = 0;
141+
bool mWasSelected = false;
142+
143+
/**
144+
* @brief A flag to account for corner cases during OTA apply
145+
*
146+
* Used by the default ApplyAction implementation.
147+
*
148+
* If something goes wrong during ExitAction of the TLV processor,
149+
* then mApplyState should be set to kDoNotApply and the image processor
150+
* should abort. In this case, the BDX transfer was already finished
151+
* and calling CancelImageUpdate will not abort the transfer, hence
152+
* the device will reboot even though it should not have. If ApplyAction
153+
* fails during HandleApply, then the process will be aborted.
154+
*/
155+
ApplyState mApplyState = ApplyState::kApply;
136156
ProcessDescriptor mCallbackProcessDescriptor = nullptr;
137157
};
138158

src/platform/nxp/k32w/k32w0/OTAFirmwareProcessor.cpp

+11-15
Original file line numberDiff line numberDiff line change
@@ -28,21 +28,21 @@ namespace chip {
2828

2929
CHIP_ERROR OTAFirmwareProcessor::Init()
3030
{
31-
ReturnErrorCodeIf(mCallbackProcessDescriptor == nullptr, CHIP_OTA_PROCESSOR_CB_NOT_REGISTERED);
31+
ReturnErrorCodeIf(mCallbackProcessDescriptor == nullptr, CHIP_ERROR_OTA_PROCESSOR_CB_NOT_REGISTERED);
3232
mAccumulator.Init(sizeof(Descriptor));
3333
#if OTA_ENCRYPTION_ENABLE
3434
mUnalignmentNum = 0;
3535
#endif
36-
ReturnErrorCodeIf(gOtaSuccess_c != OTA_ClientInit(), CHIP_OTA_PROCESSOR_CLIENT_INIT);
36+
ReturnErrorCodeIf(gOtaSuccess_c != OTA_ClientInit(), CHIP_ERROR_OTA_PROCESSOR_CLIENT_INIT);
3737

3838
auto offset = OTA_GetCurrentEepromAddressOffset();
3939
if (offset != 0)
4040
{
4141
offset += 1;
4242
}
4343

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

4747
return CHIP_NO_ERROR;
4848
}
@@ -95,7 +95,7 @@ CHIP_ERROR OTAFirmwareProcessor::ProcessInternal(ByteSpan & block)
9595
if (gOtaSuccess_c != status)
9696
{
9797
ChipLogError(SoftwareUpdate, "Failed to make room for next block. Status: %d", status);
98-
return CHIP_OTA_PROCESSOR_MAKE_ROOM;
98+
return CHIP_ERROR_OTA_PROCESSOR_MAKE_ROOM;
9999
}
100100
#if OTA_ENCRYPTION_ENABLE
101101
status = OTA_PushImageChunk((uint8_t *) mBlock.data(), (uint16_t) mBlock.size(), NULL, NULL);
@@ -105,10 +105,10 @@ CHIP_ERROR OTAFirmwareProcessor::ProcessInternal(ByteSpan & block)
105105
if (gOtaSuccess_c != status)
106106
{
107107
ChipLogError(SoftwareUpdate, "Failed to write image block. Status: %d", status);
108-
return CHIP_OTA_PROCESSOR_PUSH_CHUNK;
108+
return CHIP_ERROR_OTA_PROCESSOR_PUSH_CHUNK;
109109
}
110110

111-
return CHIP_OTA_FETCH_ALREADY_SCHEDULED;
111+
return CHIP_ERROR_OTA_FETCH_ALREADY_SCHEDULED;
112112
}
113113

114114
CHIP_ERROR OTAFirmwareProcessor::ProcessDescriptor(ByteSpan & block)
@@ -122,12 +122,6 @@ CHIP_ERROR OTAFirmwareProcessor::ProcessDescriptor(ByteSpan & block)
122122
return CHIP_NO_ERROR;
123123
}
124124

125-
CHIP_ERROR OTAFirmwareProcessor::ApplyAction()
126-
{
127-
128-
return CHIP_NO_ERROR;
129-
}
130-
131125
CHIP_ERROR OTAFirmwareProcessor::AbortAction()
132126
{
133127
OTA_CancelImage();
@@ -144,13 +138,15 @@ CHIP_ERROR OTAFirmwareProcessor::ExitAction()
144138
if (OTA_CommitImage(NULL) != gOtaSuccess_c)
145139
{
146140
ChipLogError(SoftwareUpdate, "Failed to commit firmware image.");
147-
return CHIP_OTA_PROCESSOR_IMG_COMMIT;
141+
mApplyState = ApplyState::kDoNotApply;
142+
return CHIP_ERROR_OTA_PROCESSOR_IMG_COMMIT;
148143
}
149144

150145
if (OTA_ImageAuthenticate() != gOtaImageAuthPass_c)
151146
{
152147
ChipLogError(SoftwareUpdate, "Failed to authenticate firmware image.");
153-
return CHIP_OTA_PROCESSOR_IMG_AUTH;
148+
mApplyState = ApplyState::kDoNotApply;
149+
return CHIP_ERROR_OTA_PROCESSOR_IMG_AUTH;
154150
}
155151

156152
OTA_AddNewImageFlag();

src/platform/nxp/k32w/k32w0/OTAFirmwareProcessor.h

-1
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@ class OTAFirmwareProcessor : public OTATlvProcessor
3535

3636
CHIP_ERROR Init() override;
3737
CHIP_ERROR Clear() override;
38-
CHIP_ERROR ApplyAction() override;
3938
CHIP_ERROR AbortAction() override;
4039
CHIP_ERROR ExitAction() override;
4140

src/platform/nxp/k32w/k32w1/OTAFirmwareProcessor.cpp

+9-8
Original file line numberDiff line numberDiff line change
@@ -26,16 +26,16 @@ namespace chip {
2626

2727
CHIP_ERROR OTAFirmwareProcessor::Init()
2828
{
29-
ReturnErrorCodeIf(mCallbackProcessDescriptor == nullptr, CHIP_OTA_PROCESSOR_CB_NOT_REGISTERED);
29+
ReturnErrorCodeIf(mCallbackProcessDescriptor == nullptr, CHIP_ERROR_OTA_PROCESSOR_CB_NOT_REGISTERED);
3030
mAccumulator.Init(sizeof(Descriptor));
3131

32-
ReturnErrorCodeIf(gOtaSuccess_c != OTA_SelectExternalStoragePartition(), CHIP_OTA_PROCESSOR_EXTERNAL_STORAGE);
32+
ReturnErrorCodeIf(gOtaSuccess_c != OTA_SelectExternalStoragePartition(), CHIP_ERROR_OTA_PROCESSOR_EXTERNAL_STORAGE);
3333

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

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

4040
return CHIP_NO_ERROR;
4141
}
@@ -70,7 +70,7 @@ CHIP_ERROR OTAFirmwareProcessor::ProcessInternal(ByteSpan & block)
7070
if (gOtaSuccess_c != status)
7171
{
7272
ChipLogError(SoftwareUpdate, "Failed to make room for next block. Status: %d", status);
73-
return CHIP_OTA_PROCESSOR_MAKE_ROOM;
73+
return CHIP_ERROR_OTA_PROCESSOR_MAKE_ROOM;
7474
}
7575
}
7676
else
@@ -82,10 +82,10 @@ CHIP_ERROR OTAFirmwareProcessor::ProcessInternal(ByteSpan & block)
8282
if (gOtaSuccess_c != status)
8383
{
8484
ChipLogError(SoftwareUpdate, "Failed to write image block. Status: %d", status);
85-
return CHIP_OTA_PROCESSOR_PUSH_CHUNK;
85+
return CHIP_ERROR_OTA_PROCESSOR_PUSH_CHUNK;
8686
}
8787

88-
return CHIP_OTA_FETCH_ALREADY_SCHEDULED;
88+
return CHIP_ERROR_OTA_FETCH_ALREADY_SCHEDULED;
8989
}
9090

9191
CHIP_ERROR OTAFirmwareProcessor::ProcessDescriptor(ByteSpan & block)
@@ -117,7 +117,8 @@ CHIP_ERROR OTAFirmwareProcessor::ExitAction()
117117
if (OTA_CommitImage(NULL) != gOtaSuccess_c)
118118
{
119119
ChipLogError(SoftwareUpdate, "Failed to commit firmware image.");
120-
return CHIP_OTA_PROCESSOR_IMG_COMMIT;
120+
mApplyState = ApplyState::kDoNotApply;
121+
return CHIP_ERROR_OTA_PROCESSOR_IMG_COMMIT;
121122
}
122123

123124
return CHIP_NO_ERROR;

0 commit comments

Comments
 (0)