validator_ragel: Link into TCB, use under env var

src/trusted/service_runtime/sel_validate_image.c

 /*
  * Copyright (c) 2012 The Native Client Authors. All rights reserved.
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "native_client/src/shared/platform/nacl_log.h"
 #include "native_client/src/trusted/service_runtime/sel_ldr.h"
 #include "native_client/src/trusted/validator/ncvalidate.h"
 
 const size_t kMinimumCachedCodeSize = 40000;
 
 /* Translate validation status to values wanted by sel_ldr. */
 static int NaClValidateStatus(NaClValidationStatus status) {
   switch (status) {
     case NaClValidationSucceeded:
       return LOAD_OK;
     case NaClValidationFailedOutOfMemory:
       /* Note: this is confusing, but is what sel_ldr is expecting. */
       return LOAD_BAD_FILE;
     case NaClValidationFailed:
     case NaClValidationFailedNotImplemented:
     case NaClValidationFailedCpuNotSupported:
     case NaClValidationFailedSegmentationIssue:
     default:
       return LOAD_VALIDATION_FAILED;
   }
 }
 
+typedef NaClValidationStatus (*ValidateFunc) (enum NaClSBKind,

[Nick Bray, 2012/04/19 23:11:08]

FYI, removing SBKind is next on my hit list - but I expect you'll get this
committed before I make that change.

[pasko-google - do not use, 2012/04/20 14:30:38]

not a problem

+    uintptr_t, uint8_t*, size_t, int, int, const NaClCPUFeatures*,
+    struct NaClValidationCache*);
+
 int NaClValidateCode(struct NaClApp *nap, uintptr_t guest_addr,
                      uint8_t *data, size_t size) {
   NaClValidationStatus status = NaClValidationSucceeded;
   enum NaClSBKind sb_kind = NACL_SB_DEFAULT;
-
+  const ValidateFunc cur_validate_func = NACL_SUBARCH_NAME(ApplyValidator,
+                                                           NACL_TARGET_ARCH,
+                                                           NACL_TARGET_SUBARCH);
+  const ValidateFunc dfa_validate_func = NACL_SUBARCH_NAME(ApplyDfaValidator,

[Nick Bray, 2012/04/19 23:11:08]

Don't stub this out on ARM, just ifdef out the reference.

[pasko-google - do not use, 2012/04/20 14:30:38]

across all trusted code I do not see a single #ifdef ARM, is it a good time to
break the rule? Specifically this file seems to have been designed to look as
platform-independent. If it stops to be, let's discuss design vision in more
detail. A document?

+                                                           NACL_TARGET_ARCH,
+                                                           NACL_TARGET_SUBARCH);
+  ValidateFunc validate_func = cur_validate_func;
   struct NaClValidationCache *cache = nap->validation_cache;
 
   if (size < kMinimumCachedCodeSize) {
     /*
      * Don't cache the validation of small code chunks for three reasons:
      * 1) The size of the validation cache will be bounded.  Cache entries are
      *    better used for bigger code.
      * 2) The per-transaction overhead of validation caching is more noticeable
      *    for small code.
      * 3) JITs tend to generate a lot of small code chunks, and JITed code may
      *    never be seen again.  Currently code size is the best mechanism we
      *    have for heuristically distinguishing between JIT and static code.
      *    (In practice most Mono JIT blocks are less than 1k, and a quick look
      *    didn't show any above 35k.)
      * The choice of what constitutes "small" is arbitrary, and should be
      * empirically tuned.
      * TODO(ncbray) let the syscall specify if the code is cached or not.
      */
     cache = NULL;
   }
 
   /* As fixed feature mode implies the text should be readonly, and
    * stubout mode implies updating the text, disallow their use together.
    */
   if (nap->validator_stub_out_mode && nap->fixed_feature_cpu_mode) {
     NaClLog(LOG_FATAL,
             "stub_out_mode and fixed_feature_cpu_mode are incompatible\n");
     return LOAD_VALIDATION_FAILED;
   }
+
+  if (nap->enable_dfa_validator) {

[Nick Bray, 2012/04/19 23:11:08]

Move this up to keep the validate_func selection grouped.  In fact, I'd
encapsulate the validator selection logic in a separate function.  (Running the
validator should not care where the validator comes from.)  The logic will get a
little more complicate when you take ARM into account.

Once you stick this in a function you could also create a "Warn about
experimental validator" function that is always called when you select an
experimental validator, but maintains a global Boolean (on nap?) to suppress
multiple output.  This would keep the validator selection logic grouped.  Or,
screw it, you could just warn every time the validator is called. Much simpler.
:)

I plan to eventually refactor this so that the validator is selected once at
startup and the function pointers are cached.

[pasko-google - do not use, 2012/04/20 14:30:38]

For the grouping I'd better move the validate_func selection down just before it
is used. In C it would need an extra {} block, which is well .. more or less
okay. Done.
NaClValidateCode() is already a function that selects an appropriate validating
function and additionally performs some work: run the function, check
compatibility and decide on caching. If it were a lengthy function I would fully
support the separation, but as of now it would only create another abstract
layer that complicates reading. Are there clear plans for more validation
features that require separating into multiple functions?

Choosing a validating function in a platform-dependent way would be reasonable
if we had numerous validation methods to choose from. I am trying to make things
as simple as possible (but not simpler). I.e. making a nice extensible
infrastructure for something that is not going to be extended is something I do
not support.
Take a different look: the two (three?) validators will be there simultaneously
for some time, then the dispatch function will have to be reverted because it
would only make a choice among one option.
If it is for speedup, then it is overly premature. Global state like that might
look more elegant than now, but not for a large extent, so I would not bother.

+    validate_func = dfa_validate_func;
+  }
+
   if (nap->validator_stub_out_mode) {
     /* Validation caching is currently incompatible with stubout. */
     cache = NULL;
     /* In stub out mode, we do two passes.  The second pass acts as a
        sanity check that bad instructions were indeed overwritten with
        allowable HLTs. */
-    status = NACL_SUBARCH_NAME(ApplyValidator,
-                               NACL_TARGET_ARCH,
-                               NACL_TARGET_SUBARCH)(
-                                   sb_kind,
-                                   guest_addr, data, size,
-                                   TRUE, /* stub out */
-                                   FALSE, /* text is not read-only */
-                                   &nap->cpu_features,
-                                   cache);
+    status = validate_func(sb_kind,
+                           guest_addr, data, size,
+                           TRUE, /* stub out */
+                           FALSE, /* text is not read-only */
+                           &nap->cpu_features,
+                           cache);
   }
   if (status == NaClValidationSucceeded) {
-    /* Fixed feature CPU mode implies read-only */
+    /* Fixed feature CPU mode implies read-only. */
     int readonly_text = nap->fixed_feature_cpu_mode;
-    status = NACL_SUBARCH_NAME(ApplyValidator,
-                               NACL_TARGET_ARCH,
-                               NACL_TARGET_SUBARCH)(
-                                   sb_kind,
-                                   guest_addr, data, size,
-                                   FALSE, /* do not stub out */
-                                   readonly_text,
-                                   &nap->cpu_features,
-                                   cache);
+    status = validate_func(sb_kind,
+                           guest_addr, data, size,
+                           FALSE, /* do not stub out */
+                           readonly_text,
+                           &nap->cpu_features,
+                           cache);
   }
   return NaClValidateStatus(status);
 }
 
 int NaClValidateCodeReplacement(struct NaClApp *nap, uintptr_t guest_addr,
                                 uint8_t *data_old, uint8_t *data_new,
                                 size_t size) {
   enum NaClSBKind sb_kind = NACL_SB_DEFAULT;
   if (nap->validator_stub_out_mode) return LOAD_BAD_FILE;
   if (nap->fixed_feature_cpu_mode) return LOAD_BAD_FILE;
@@ skipping 55 matching lines @@
         NaClLog(LOG_ERROR,
                 "Run sel_ldr in debug mode to ignore validation failure.\n");
         NaClLog(LOG_ERROR,
                 "Run ncval <module-name> for validation error details.\n");
         rcode = LOAD_VALIDATION_FAILED;
       }
     }
   }
   return rcode;
 }