validator_ragel: Link into TCB, use under env var

src/trusted/service_runtime/sel_validate_image.c

Index: src/trusted/service_runtime/sel_validate_image.c
diff --git a/src/trusted/service_runtime/sel_validate_image.c b/src/trusted/service_runtime/sel_validate_image.c
index 1875ac1fe572aa87474d385ae73c5e5968472472..d0d135523256de5007cea1c273c7fd3a989da55b 100644
--- a/src/trusted/service_runtime/sel_validate_image.c
+++ b/src/trusted/service_runtime/sel_validate_image.c
@@ -27,11 +27,21 @@ static int NaClValidateStatus(NaClValidationStatus status) {
   }
 }
 
+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) {
@@ -61,34 +71,33 @@ int NaClValidateCode(struct NaClApp *nap, uintptr_t guest_addr,
             "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);
 }