keymaster: correctly compute consumed count

Update calls did not correctly calculate the
consumed count, and the HAL would consume
more than could be sent to citadel.  As a
result the final call would send the balance
and overflow rpc buffers.

Bug: 118404627
Test: VTS tests pass
Change-Id: I8d105122ed4344c1517e8c6124c93d1eb9b6e922
(cherry picked from commit 9430f730e342a512536f060fe3afc174c12cb98c)

3 files changed, 58 insertions, 21 deletions

diff --git a/hals/keymaster/KeymasterDevice.cpp b/hals/keymaster/KeymasterDevice.cpp
index 3c569a0..daa17ee 100644
--- a/hals/keymaster/KeymasterDevice.cpp
+++ b/hals/keymaster/KeymasterDevice.cpp
@@ -28,6 +28,8 @@
 
 #include <keymasterV4_0/key_param_output.h>
 
+#include <openssl/sha.h>
+
 #include <android-base/logging.h>
 #include <android-base/properties.h>
 
@@ -259,6 +261,24 @@ static ErrorCode status_to_error_code(uint32_t status)
     }                                                                         \
 }
 
+#define KM_CALLV_ABORT(meth, request, response, ...) {                        \
+    const uint32_t status = _keymaster. meth (request, &response);            \
+    const ErrorCode error_code = translate_error_code(response.error_code()); \
+    if (status != APP_SUCCESS) {                                              \
+        LOG(ERROR) << #meth << " : request failed with status: "              \
+                   << nos::StatusCodeString(status) << " aborting operation"; \
+        _hidl_cb(status_to_error_code(status), __VA_ARGS__);                  \
+        abort(request.handle().handle());                                     \
+        return Void();                                                        \
+    }                                                                         \
+    if (error_code != ErrorCode::OK) {                                        \
+        LOG(ERROR) << #meth << " : device response error code: "              \
+                   << error_code;                                             \
+        _hidl_cb(error_code, __VA_ARGS__);                                    \
+        return Void();                                                        \
+    }                                                                         \
+}
+
 // Methods from ::android::hardware::keymaster::V3_0::IKeymasterDevice follow.
 
 KeymasterDevice::KeymasterDevice(KeymasterClient& keymaster) :
@@ -1017,19 +1037,6 @@ Return<void> KeymasterDevice::update(
     UpdateOperationRequest request;
     UpdateOperationResponse response;
 
-    // TODO: does keystore chunk stream data?  To what quantum?
-    if (input.size() > KM_MAX_PROTO_FIELD_SIZE) {
-        LOG(ERROR) << "Excess input length: " << input.size()
-                   << " max allowed: " << KM_MAX_PROTO_FIELD_SIZE;
-        if (this->abort(operationHandle) != ErrorCode::OK) {
-            LOG(ERROR) << "abort( " << operationHandle
-                       << ") failed";
-        }
-        _hidl_cb(ErrorCode::INVALID_INPUT_LENGTH, 0,
-                 hidl_vec<KeyParameter>{}, hidl_vec<uint8_t>{});
-        return Void();
-    }
-
     uint32_t consumed;
     hidl_vec<uint8_t> output;
     hidl_vec<KeyParameter> params;
@@ -1068,8 +1075,8 @@ Return<void> KeymasterDevice::update(
     translate_verification_token(verificationToken,
                                  request.mutable_verification_token());
 
-    KM_CALLV(UpdateOperation, request, response,
-             0, hidl_vec<KeyParameter>{}, hidl_vec<uint8_t>{});
+    KM_CALLV_ABORT(UpdateOperation, request, response,
+                   0, hidl_vec<KeyParameter>{}, hidl_vec<uint8_t>{});
 
     if (buffer_advance(operationHandle, response.consumed()) != ErrorCode::OK) {
         _hidl_cb(ErrorCode::UNKNOWN_ERROR, 0, params, output);
@@ -1081,6 +1088,16 @@ Return<void> KeymasterDevice::update(
         reinterpret_cast<uint8_t*>(const_cast<char*>(response.output().data())),
         response.output().size(), false);
 
+    // Special case ECDSA sign + Digest::NONE, which discards all but
+    // the left-most len(SHA256) bytes.
+    Algorithm algorithm;
+    buffer_algorithm(operationHandle, &algorithm);
+    if (algorithm == Algorithm::EC) {
+        if (response.consumed() == 0 && // Implies Digest::NONE.
+            buffer_remaining(operationHandle) >= SHA256_DIGEST_LENGTH) {
+            consumed = input.size();    // Discard remaining input.
+        }
+    }
     _hidl_cb(ErrorCode::OK, consumed, params, output);
     return Void();
 }
@@ -1149,8 +1166,8 @@ Return<void> KeymasterDevice::finish(
     translate_verification_token(verificationToken,
                                  request.mutable_verification_token());
 
-    KM_CALLV(FinishOperation, request, response,
-             hidl_vec<KeyParameter>{}, hidl_vec<uint8_t>{});
+    KM_CALLV_ABORT(FinishOperation, request, response,
+                   hidl_vec<KeyParameter>{}, hidl_vec<uint8_t>{});
 
     pb_to_hidl_params(response.params(), &params);
     output.setToExternal(
diff --git a/hals/keymaster/buffer.cpp b/hals/keymaster/buffer.cpp
index 05e9c3e..35a7d4f 100644
--- a/hals/keymaster/buffer.cpp
+++ b/hals/keymaster/buffer.cpp
@@ -39,7 +39,8 @@ using std::map;
 using std::pair;
 using std::vector;
 
-static const size_t kMaxChunkSize = 256;
+/* Stack space constrains the input/output size to RSA_MAX_BYTES (3k) */
+static const size_t kMaxChunkSize = 384;
 
 class Operation {
 public:
@@ -65,8 +66,10 @@ public:
     }
 
     void append(const hidl_vec<uint8_t>& input, uint32_t *consumed) {
-        _buffer.insert(_buffer.end(), input.begin(), input.end());
-        *consumed = input.size();
+        const size_t count = std::min(
+            kMaxChunkSize - _buffer.size(), input.size());
+        _buffer.insert(_buffer.end(), input.begin(), input.begin() + count);
+        *consumed = count;
     }
 
     void peek(hidl_vec<uint8_t> *data) {
@@ -84,7 +87,7 @@ public:
         } else {
             retain = (_buffer.size() % _blockSize) + _blockSize;
         }
-        const size_t count = std::min(_buffer.size() - retain, kMaxChunkSize);
+        const size_t count = _buffer.size() - retain;
         *data = vector<uint8_t>(_buffer.begin(), _buffer.begin() + count);
     }
 
@@ -105,6 +108,10 @@ public:
         _buffer.clear();
     }
 
+    Algorithm algorithm(void) {
+        return _algorithm;
+    }
+
 private:
     Algorithm _algorithm;
     size_t _blockSize;
@@ -190,6 +197,17 @@ ErrorCode buffer_final(uint64_t handle,
     return ErrorCode::OK;
 }
 
+ErrorCode buffer_algorithm(uint64_t handle, Algorithm *algorithm)
+{
+    if (buffer_map.find(handle) == buffer_map.end()) {
+        LOG(ERROR) << "Algorithm requested on absent operation: " << handle;
+        return ErrorCode::UNKNOWN_ERROR;
+    }
+    Operation *op = &buffer_map.find(handle)->second;
+    *algorithm = op->algorithm();
+    return ErrorCode::OK;
+}
+
 }  // namespace keymaster
 }  // hardware
 }  // android
diff --git a/hals/keymaster/buffer.h b/hals/keymaster/buffer.h
index a4a23e3..4fc7468 100644
--- a/hals/keymaster/buffer.h
+++ b/hals/keymaster/buffer.h
@@ -41,6 +41,8 @@ ErrorCode buffer_peek(uint64_t handle,
 ErrorCode buffer_advance(uint64_t handle, size_t count);
 ErrorCode buffer_final(uint64_t handle,
                        hidl_vec<uint8_t> *data);
+ErrorCode buffer_algorithm(uint64_t handle,
+                           Algorithm *algorithm);
 
 }  // namespace keymaster
 }  // hardware