Hal review comment fixes

HAL/CborConverter.h

@@ -15,18 +15,22 @@
  ** limitations under the License.
  */
 #pragma once
-#include <aidl/android/hardware/security/keymint/Certificate.h>
-#include <aidl/android/hardware/security/keymint/IKeyMintDevice.h>
-#include <aidl/android/hardware/security/secureclock/TimeStampToken.h>
-#include <aidl/android/hardware/security/sharedsecret/ISharedSecret.h>
-#include <cppbor.h>
-#include <cppbor_parse.h>
+
 #include <iostream>
-#include <keymaster/android_keymaster_messages.h>
 #include <memory>
 #include <numeric>
 #include <vector>
 
+#include <cppbor.h>
+#include <cppbor_parse.h>
+
+#include <aidl/android/hardware/security/keymint/Certificate.h>
+#include <aidl/android/hardware/security/keymint/IKeyMintDevice.h>
+#include <aidl/android/hardware/security/sharedsecret/ISharedSecret.h>
+#include <aidl/android/hardware/security/secureclock/TimeStampToken.h>
+
+#include <keymaster/android_keymaster_messages.h>
+
 namespace keymint::javacard {
 using namespace cppbor;
 using namespace aidl::android::hardware::security::keymint;
@@ -44,22 +48,35 @@ class CborConverter {
  decodeData(const std::vector<uint8_t>& response);
 
  template <typename T>
- bool getUint64(const std::unique_ptr<Item>& item, const uint32_t pos, T& value);
-
- template <typename T> bool getUint64(const std::unique_ptr<Item>& item, T& value);
+ std::optional<T> getUint64(const unique_ptr<Item> &item) {
+ T value;
+ if ((item == nullptr) || (std::is_unsigned<T>::value && (MajorType::UINT != getType(item))) ||
+ ((std::is_signed<T>::value && (MajorType::NINT != getType(item))))) {
+ return std::nullopt;
+ }
+ if (std::is_unsigned<T>::value) {
+ const Uint *uintVal = item.get()->asUint();
+ value = static_cast<T>(uintVal->value());
+ } else {
+ const Nint *nintVal = item.get()->asNint();
+ value = static_cast<T>(nintVal->value());
+ }
+ return value; // success
+ }
 
- bool getSharedSecretParameters(const std::unique_ptr<Item>& item, const uint32_t pos,
- SharedSecretParameters& params);
- bool getBinaryArray(const std::unique_ptr<Item>& item, const uint32_t pos, string& value);
+ template <typename T>
+ std::optional<T> getUint64(const unique_ptr<Item> &item, const uint32_t pos) {
+ auto intItem = getItemAtPos(item, pos);
+ return getUint64<T>(intItem.value());
+ }
 
- bool getBinaryArray(const std::unique_ptr<Item>& item, const uint32_t pos,
- vector<uint8_t>& value);
+ std::optional<SharedSecretParameters> getSharedSecretParameters(const std::unique_ptr<Item>& item, const uint32_t pos);
+
+ std::optional<string> getByteArrayStr(const unique_ptr<Item>& item, const uint32_t pos);
 
- bool getHardwareAuthToken(const std::unique_ptr<Item>& item, const uint32_t pos,
- HardwareAuthToken& authType);
+ std::optional<std::vector<uint8_t>> getByteArrayVec(const unique_ptr<Item>& item, const uint32_t pos);
 
- bool getKeyParameters(const std::unique_ptr<Item>& item, const uint32_t pos,
- vector<KeyParameter>& keyParams);
+ std::optional<vector<KeyParameter>> getKeyParameters(const unique_ptr<Item>& item, const uint32_t pos);
 
  bool addKeyparameters(Array& array, const vector<KeyParameter>& keyParams);
 
@@ -69,34 +86,27 @@ class CborConverter {
 
  bool addSharedSecretParameters(Array& array, const vector<SharedSecretParameters>& params);
 
- bool getTimeStampToken(const std::unique_ptr<Item>& item, const uint32_t pos,
- TimeStampToken& token);
+ std::optional<TimeStampToken> getTimeStampToken(const std::unique_ptr<Item>& item, const uint32_t pos);
 
- bool getKeyCharacteristics(const std::unique_ptr<Item>& item, const uint32_t pos,
- vector<KeyCharacteristics>& keyCharacteristics);
+ std::optional<vector<KeyCharacteristics>> getKeyCharacteristics(const std::unique_ptr<Item>& item, const uint32_t pos);
 
- bool getCertificateChain(const std::unique_ptr<Item>& item, const uint32_t pos,
- vector<Certificate>& keyCharacteristics);
+ std::optional<vector<Certificate>> getCertificateChain(const std::unique_ptr<Item>& item, const uint32_t pos);
 
- bool getMultiBinaryArray(const std::unique_ptr<Item>& item, const uint32_t pos,
- vector<vector<uint8_t>>& data);
+ std::optional<vector<vector<uint8_t>>> getMultiByteArray(const unique_ptr<Item>& item, const uint32_t pos);
 
  bool addTimeStampToken(Array& array, const TimeStampToken& token);
 
- bool getMapItem(const std::unique_ptr<Item>& item, const uint32_t pos,
- Map& map);
+ std::optional<Map> getMapItem(const std::unique_ptr<Item>& item, const uint32_t pos);
 
- bool getArrayItem(const std::unique_ptr<Item>& item, const uint32_t pos,
- Array& array);
-
- inline bool getErrorCode(const std::unique_ptr<Item>& item, const uint32_t pos,
- keymaster_error_t& errorCode) {
- uint64_t errorVal;
- if (!getUint64<uint64_t>(item, pos, errorVal)) {
- return false;
+ std::optional<Array> getArrayItem(const std::unique_ptr<Item>& item, const uint32_t pos);
+
+ inline std::optional<keymaster_error_t> getErrorCode(const std::unique_ptr<Item>& item, const uint32_t pos) {
+
+ auto optErrorVal = getUint64<uint64_t>(item, pos);
+ if (!optErrorVal) {
+ return std::nullopt;
  }
- errorCode = static_cast<keymaster_error_t>(0 - errorVal);
- return true;
+ return static_cast<keymaster_error_t>(0 - optErrorVal.value());
  }
 
  private:
@@ -115,49 +125,25 @@ class CborConverter {
  * value contains binary string. If TagType is UINT_REP or ULONG_REP the value contains Array of
  * unsigned integers.
  */
- bool getKeyParameter(const std::pair<const unique_ptr<Item>&, const unique_ptr<Item>&> pair,
- vector<KeyParameter>& keyParam);
+ std::optional<std::vector<KeyParameter>>
+ getKeyParameter(const std::pair<const std::unique_ptr<Item>&,
+ const std::unique_ptr<Item>&> pair);
 
  /**
  * Get the sub item pointer from the root item pointer at the given position.
  */
- inline void getItemAtPos(const unique_ptr<Item>& item, const uint32_t pos,
- unique_ptr<Item>& subItem) {
+ inline std::optional<unique_ptr<Item>> getItemAtPos(const unique_ptr<Item>& item, const uint32_t pos) {
  Array* arr = nullptr;
 
  if (MajorType::ARRAY != getType(item)) {
- return;
+ return std::nullopt;
  }
  arr = const_cast<Array*>(item.get()->asArray());
  if (arr->size() < (pos + 1)) {
- return;
+ return std::nullopt;
  }
-subItem = std::move((*arr)[pos]);
+return std::move((*arr)[pos]);
  }
 };
 
-template <typename T> bool CborConverter::getUint64(const unique_ptr<Item>& item, T& value) {
- bool ret = false;
- if ((item == nullptr) || (std::is_unsigned<T>::value && (MajorType::UINT != getType(item))) ||
- ((std::is_signed<T>::value && (MajorType::NINT != getType(item))))) {
- return ret;
- }
-
- if (std::is_unsigned<T>::value) {
- const Uint* uintVal = item.get()->asUint();
- value = static_cast<T>(uintVal->value());
- } else {
- const Nint* nintVal = item.get()->asNint();
- value = static_cast<T>(nintVal->value());
- }
- ret = true;
- return ret; // success
-}
-
-template <typename T>
-bool CborConverter::getUint64(const unique_ptr<Item>& item, const uint32_t pos, T& value) {
- unique_ptr<Item> intItem(nullptr);
- getItemAtPos(item, pos, intItem);
- return getUint64(intItem, value);
-}
 } // namespace keymint::javacard

HAL/ITransport.h

@@ -15,8 +15,10 @@
  ** limitations under the License.
  */
 #pragma once
+
 #include <memory>
 #include <vector>
+
 #include <hardware/keymaster_defs.h>
 
 namespace keymint::javacard {

HAL/JavacardKeyMintDevice.cpp

@@ -15,23 +15,28 @@
  */
 
 #define LOG_TAG "javacard.keymint.device.strongbox-impl"
+
 #include "JavacardKeyMintDevice.h"
-#include "JavacardKeyMintOperation.h"
-#include "JavacardSharedSecret.h"
-#include <JavacardKeyMintUtils.h>
+
+#include <regex.h>
+
 #include <algorithm>
-#include <android-base/logging.h>
-#include <android-base/properties.h>
-#include <hardware/hw_auth_token.h>
 #include <iostream>
 #include <iterator>
-#include <keymaster/android_keymaster_messages.h>
-#include <keymaster/wrapped_key.h>
 #include <memory>
-#include <regex.h>
 #include <string>
 #include <vector>
 
+#include <android-base/logging.h>
+#include <android-base/properties.h>
+#include <hardware/hw_auth_token.h>
+#include <keymaster/android_keymaster_messages.h>
+#include <keymaster/wrapped_key.h>
+
+#include "JavacardKeyMintOperation.h"
+#include "JavacardKeyMintUtils.h"
+#include "JavacardSharedSecret.h"
+
 namespace aidl::android::hardware::security::keymint {
 using km_utils::KmParamSet;
 using namespace ::keymaster;
@@ -48,24 +53,28 @@ ScopedAStatus JavacardKeyMintDevice::defaultHwInfo(KeyMintHardwareInfo* info) {
 
 
 ScopedAStatus JavacardKeyMintDevice::getHardwareInfo(KeyMintHardwareInfo* info) {
- uint64_t tsRequired = 1;
  auto [item, err] = card_->sendRequest(Instruction::INS_GET_HW_INFO_CMD);
- uint32_t secLevel;
- uint32_t version;
- if (err != KM_ERROR_OK || !cbor_.getUint64<uint32_t>(item, 1, version) ||
- !cbor_.getUint64<uint32_t>(item, 2, secLevel) ||
- !cbor_.getBinaryArray(item, 3, info->keyMintName) ||
- !cbor_.getBinaryArray(item, 4, info->keyMintAuthorName) ||
- !cbor_.getUint64<uint64_t>(item, 5, tsRequired)) {
+ std::optional<string> optKeyMintName;
+ std::optional<string> optKeyMintAuthorName;
+ std::optional<uint32_t> optSecLevel;
+ std::optional<uint32_t> optVersion;
+ std::optional<uint64_t> optTsRequired;
+ if (err != KM_ERROR_OK || !(optVersion = cbor_.getUint64<uint32_t>(item, 1)) ||
+ !(optSecLevel = cbor_.getUint64<uint32_t>(item, 2)) ||
+ !(optKeyMintName = cbor_.getByteArrayStr(item, 3)) ||
+ !(optKeyMintAuthorName = cbor_.getByteArrayStr(item, 4)) ||
+ !(optTsRequired = cbor_.getUint64<uint64_t>(item, 5))) {
  // TODO should we return HARDWARE_NOT_YET_AVAILABLE instead of default Hardware Info.
  LOG(ERROR) << "Error in response of getHardwareInfo.";
  LOG(INFO) << "Returning defaultHwInfo in getHardwareInfo.";
  return defaultHwInfo(info);
  }
  card_->initializeJavacard();
- info->timestampTokenRequired = (tsRequired == 1);
- info->securityLevel = static_cast<SecurityLevel>(secLevel);
- info->versionNumber = static_cast<int32_t>(version);
+ info->keyMintName = std::move(optKeyMintName.value());
+ info->keyMintAuthorName = std::move(optKeyMintAuthorName.value());
+ info->timestampTokenRequired = (optTsRequired.value() == 1);
+ info->securityLevel = static_cast<SecurityLevel>(std::move(optSecLevel.value()));
+ info->versionNumber = static_cast<int32_t>(std::move(optVersion.value()));
  return ScopedAStatus::ok();
 }
 
@@ -82,12 +91,16 @@ ScopedAStatus JavacardKeyMintDevice::generateKey(const vector<KeyParameter>& key
  LOG(ERROR) << "Error in sending generateKey.";
  return km_utils::kmError2ScopedAStatus(err);
  }
- if (!cbor_.getBinaryArray(item, 1, creationResult->keyBlob) ||
- !cbor_.getKeyCharacteristics(item, 2, creationResult->keyCharacteristics) ||
- !cbor_.getCertificateChain(item, 3, creationResult->certificateChain)) {
+ auto optKeyBlob = cbor_.getByteArrayVec(item, 1);
+ auto optKeyChars = cbor_.getKeyCharacteristics(item, 2);
+ auto optCertChain = cbor_.getCertificateChain(item, 3);
+ if (!optKeyBlob || !optKeyChars || !optCertChain) {
  LOG(ERROR) << "Error in decoding og response in generateKey.";
  return km_utils::kmError2ScopedAStatus(KM_ERROR_UNKNOWN_ERROR);
  }
+ creationResult->keyCharacteristics = std::move(optKeyChars.value());
+ creationResult->certificateChain = std::move(optCertChain.value());
+ creationResult->keyBlob = std::move(optKeyBlob.value());
  return ScopedAStatus::ok();
 }
 
@@ -123,12 +136,16 @@ ScopedAStatus JavacardKeyMintDevice::importKey(const vector<KeyParameter>& keyPa
  LOG(ERROR) << "Error in sending data in importKey.";
  return km_utils::kmError2ScopedAStatus(err);
  }
- if (!cbor_.getBinaryArray(item, 1, creationResult->keyBlob) ||
- !cbor_.getKeyCharacteristics(item, 2, creationResult->keyCharacteristics) ||
- !cbor_.getCertificateChain(item, 3, creationResult->certificateChain)) {
+ auto optKeyBlob = cbor_.getByteArrayVec(item, 1);
+ auto optKeyChars = cbor_.getKeyCharacteristics(item, 2);
+ auto optCertChain = cbor_.getCertificateChain(item, 3);
+ if (!optKeyBlob || !optKeyChars || !optCertChain) {
  LOG(ERROR) << "Error in decoding response in importKey.";
  return km_utils::kmError2ScopedAStatus(KM_ERROR_UNKNOWN_ERROR);
  }
+ creationResult->keyCharacteristics = std::move(optKeyChars.value());
+ creationResult->certificateChain = std::move(optCertChain.value());
+ creationResult->keyBlob = std::move(optKeyBlob.value());
  return ScopedAStatus::ok();
 }
 
@@ -172,12 +189,16 @@ ScopedAStatus JavacardKeyMintDevice::importWrappedKey(const vector<uint8_t>& wra
  LOG(ERROR) << "Error in send finish import wrapped key in importWrappedKey.";
  return km_utils::kmError2ScopedAStatus(errorCode);
  }
- if (!cbor_.getBinaryArray(item, 1, creationResult->keyBlob) ||
- !cbor_.getKeyCharacteristics(item, 2, creationResult->keyCharacteristics) ||
- !cbor_.getCertificateChain(item, 3, creationResult->certificateChain)) {
+ auto optKeyBlob = cbor_.getByteArrayVec(item, 1);
+ auto optKeyChars = cbor_.getKeyCharacteristics(item, 2);
+ auto optCertChain = cbor_.getCertificateChain(item, 3);
+ if (!optKeyBlob || !optKeyChars || !optCertChain) {
  LOG(ERROR) << "Error in decoding the response in importWrappedKey.";
  return km_utils::kmError2ScopedAStatus(KM_ERROR_UNKNOWN_ERROR);
  }
+ creationResult->keyCharacteristics = std::move(optKeyChars.value());
+ creationResult->certificateChain = std::move(optCertChain.value());
+ creationResult->keyBlob = std::move(optKeyBlob.value());
  return ScopedAStatus::ok();
 }
 
@@ -225,10 +246,12 @@ ScopedAStatus JavacardKeyMintDevice::upgradeKey(const vector<uint8_t>& keyBlobTo
  LOG(ERROR) << "Error in sending in upgradeKey.";
  return km_utils::kmError2ScopedAStatus(err);
  }
- if (!cbor_.getBinaryArray(item, 1, *keyBlob)) {
+ auto optKeyBlob = cbor_.getByteArrayVec(item, 1);
+ if (!optKeyBlob) {
  LOG(ERROR) << "Error in decoding the response in upgradeKey.";
  return km_utils::kmError2ScopedAStatus(KM_ERROR_UNKNOWN_ERROR);
  }
+ *keyBlob = std::move(optKeyBlob.value());
  return ScopedAStatus::ok();
 }
 
@@ -284,20 +307,20 @@ ScopedAStatus JavacardKeyMintDevice::begin(KeyPurpose purpose, const std::vector
  return km_utils::kmError2ScopedAStatus(err);
  }
  // return the result
- uint64_t opHandle;
- uint8_t bufMode;
- uint16_t macLength;
- if (!cbor_.getKeyParameters(item, 1, result->params) ||
- !cbor_.getUint64<uint64_t>(item, 2, opHandle) ||
- !cbor_.getUint64<uint8_t>(item, 3, bufMode) ||
- !cbor_.getUint64<uint16_t>(item, 4, macLength)) {
+ auto keyParams = cbor_.getKeyParameters(item, 1);
+ auto optOpHandle = cbor_.getUint64<uint64_t>(item, 2);
+ auto optBufMode = cbor_.getUint64<uint8_t>(item, 3);
+ auto optMacLength = cbor_.getUint64<uint16_t>(item, 4);
+
+ if (!keyParams || !optOpHandle || !optBufMode || !optMacLength) {
  LOG(ERROR) << "Error in decoding the response in begin.";
  return km_utils::kmError2ScopedAStatus(KM_ERROR_UNKNOWN_ERROR);
  }
- result->challenge = opHandle;
+ result->params = std::move(keyParams.value());
+ result->challenge = optOpHandle.value();
  result->operation = ndk::SharedRefBase::make<JavacardKeyMintOperation>(
- static_cast<keymaster_operation_handle_t>(opHandle), static_cast<BufferingMode>(bufMode),
-macLength, card_);
+ static_cast<keymaster_operation_handle_t>(optOpHandle.value()), static_cast<BufferingMode>(optBufMode.value()),
+optMacLength.value(), card_);
  return ScopedAStatus::ok();
 }
 
@@ -350,10 +373,12 @@ ScopedAStatus JavacardKeyMintDevice::getKeyCharacteristics(
  LOG(ERROR) << "Error in sending in getKeyCharacteristics.";
  return km_utils::kmError2ScopedAStatus(err);
  }
- if (!cbor_.getKeyCharacteristics(item, 1, *result)) {
+ auto optKeyChars = cbor_.getKeyCharacteristics(item, 1);
+ if (!optKeyChars) {
  LOG(ERROR) << "Error in sending in upgradeKey.";
  return km_utils::kmError2ScopedAStatus(KM_ERROR_UNKNOWN_ERROR);
  }
+ *result = std::move(optKeyChars.value());
  return ScopedAStatus::ok();
 }