[xen-devel] [fuzz] [x86 emulator] Input size

[xen-devel] [fuzz] [x86 emulator] Input size

[Paul Semel]

Hello,


In the x86 instruction emulator fuzzer, when checking wether the input size is 
correct, we are checking for this bounds : DATA_OFFSET < size < INPUT_SIZE.


The fact is that INPUT_SIZE is actually the size of the data buffer in the 
fuzz_corpus structure. This way, AFL is not able to have full control over this 
entry, as we are actually filling this buffer for at most
INPUT_SIZE - DATA_OFFSET.


If I understand the fuzzer correctly, we really need to give full control on 
this to AFL so that we can get some "random" from it.


I am wondering if the bounds should rather be :
DATA_OFFSET < size < sizeof (struct fuzz_corpus)
but maybe I am missing something here 🙂



Thanks,

-- 
Paul Semel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [xen-devel] [fuzz] [x86 emulator] Input size

[Wei Liu]

On Thu, Feb 22, 2018 at 01:39:01PM +0100, Paul Semel wrote:
> Hello,
> 
> 
> In the x86 instruction emulator fuzzer, when checking wether the input size
> is correct, we are checking for this bounds : DATA_OFFSET < size <
> INPUT_SIZE.
> 
> 
> The fact is that INPUT_SIZE is actually the size of the data buffer in the
> fuzz_corpus structure. This way, AFL is not able to have full control over
> this entry, as we are actually filling this buffer for at most
> INPUT_SIZE - DATA_OFFSET.
> 
> 
> If I understand the fuzzer correctly, we really need to give full control on
> this to AFL so that we can get some "random" from it.
> 
> 
> I am wondering if the bounds should rather be :
> DATA_OFFSET < size < sizeof (struct fuzz_corpus)

Yes. I think you're right.

The code has gone through several iterations. It isn't surprising that
we got something messed up in between.

(CC'ing people who modified the code before to sanity-check)

Are you up for writing a patch? I.e. replacing INPUT_SIZE with a sizeof
expression.

Also I think the BUILD_BUG_ON in fuzz_minimal_input_size should also be
fixed in a similar fashion.

Wei.

> but maybe I am missing something here 🙂
> 
> 
> 
> Thanks,
> 
> -- 
> Paul Semel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH] fuzz/x86_emulate: fix bounds for input size

[Paul Semel]

The minimum size for the input size was set to DATA_OFFSET + 1 which was meaning
that we were requesting at least one character of the data array to be filled.
This is not needed for the fuzzer to get working correctly.

The maximum size for the input size was set to INPUT_SIZE, which is actually
the size of the data array inside the fuzz_corpus structure and so was not
abling user (or AFL) to fill in the whole structure. Changing to
sizeof(struct fuzz_corpus) correct this problem.

Signed-off-by: Paul Semel <semelpaul@gmail.com>
---
 tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index 964682aa1a..f3ce2e7e27 100644
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -33,6 +33,7 @@ struct fuzz_corpus
     unsigned char data[INPUT_SIZE];
 } input;
 #define DATA_OFFSET offsetof(struct fuzz_corpus, data)
+#define FUZZ_CORPUS_SIZE (sizeof(struct fuzz_corpus))
 
 /*
  * Internal state of the fuzzing harness.  Calculated initially from the input
@@ -822,13 +823,13 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
     /* Reset all global state variables */
     memset(&input, 0, sizeof(input));
 
-    if ( size <= DATA_OFFSET )
+    if ( size < DATA_OFFSET )
     {
         printf("Input too small\n");
         return 1;
     }
 
-    if ( size > INPUT_SIZE )
+    if ( size > FUZZ_CORPUS_SIZE )
     {
         printf("Input too large\n");
         return 1;
@@ -859,9 +860,9 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
 
 unsigned int fuzz_minimal_input_size(void)
 {
-    BUILD_BUG_ON(DATA_OFFSET > INPUT_SIZE);
+    BUILD_BUG_ON(DATA_OFFSET > FUZZ_CORPUS_SIZE);
 
-    return DATA_OFFSET + 1;
+    return DATA_OFFSET;
 }
 
 /*
-- 
2.16.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] fuzz/x86_emulate: fix bounds for input size

[Paul Semel]

On 02/23/2018 12:57 AM, Paul Semel wrote:
> The minimum size for the input size was set to DATA_OFFSET + 1 which was meaning
> that we were requesting at least one character of the data array to be filled.
> This is not needed for the fuzzer to get working correctly.
> 
> The maximum size for the input size was set to INPUT_SIZE, which is actually
> the size of the data array inside the fuzz_corpus structure and so was not
> abling user (or AFL) to fill in the whole structure. Changing to
> sizeof(struct fuzz_corpus) correct this problem.
> 
> Signed-off-by: Paul Semel <semelpaul@gmail.com>
> ---
>   tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> index 964682aa1a..f3ce2e7e27 100644
> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> @@ -33,6 +33,7 @@ struct fuzz_corpus
>       unsigned char data[INPUT_SIZE];
>   } input;
>   #define DATA_OFFSET offsetof(struct fuzz_corpus, data)
> +#define FUZZ_CORPUS_SIZE (sizeof(struct fuzz_corpus))
>   
>   /*
>    * Internal state of the fuzzing harness.  Calculated initially from the input
> @@ -822,13 +823,13 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
>       /* Reset all global state variables */
>       memset(&input, 0, sizeof(input));
>   
> -    if ( size <= DATA_OFFSET )
> +    if ( size < DATA_OFFSET )
>       {
>           printf("Input too small\n");
>           return 1;
>       }
>   
> -    if ( size > INPUT_SIZE )
> +    if ( size > FUZZ_CORPUS_SIZE )
>       {
>           printf("Input too large\n");
>           return 1;
> @@ -859,9 +860,9 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
>   
>   unsigned int fuzz_minimal_input_size(void)
>   {
> -    BUILD_BUG_ON(DATA_OFFSET > INPUT_SIZE);
> +    BUILD_BUG_ON(DATA_OFFSET > FUZZ_CORPUS_SIZE);

Actually, this check is dumb.

I think I should rather do :
	BUILD_BUG_ON(DATA_OFFSET + INPUT_SIZE != FUZZ_CORPUS_SIZE)

This way, we ensure that the data field is the last field of fuzz_corpus 
structure.

What do you think about this ?

>   
> -    return DATA_OFFSET + 1;
> +    return DATA_OFFSET;
>   }
>   
>   /*
> 

-- 
Paul Semel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] fuzz/x86_emulate: fix bounds for input size

[George Dunlap]

Paul, thanks for reporting this!  A couple of comments...

On 02/22/2018 11:57 PM, Paul Semel wrote:
> The minimum size for the input size was set to DATA_OFFSET + 1 which was meaning
> that we were requesting at least one character of the data array to be filled.
> This is not needed for the fuzzer to get working correctly.

Sure, but then the emulator will fail the first insn_fetch() callback.
I understand there's a conceptual purity to testing such an input, but
is it actually of practical value in finding bugs?

> The maximum size for the input size was set to INPUT_SIZE, which is actually
> the size of the data array inside the fuzz_corpus structure and so was not
> abling user (or AFL) to fill in the whole structure. Changing to
> sizeof(struct fuzz_corpus) correct this problem.

Good catch; actually INPUT_SIZE is a misnomer; this should really be
DATA_SIZE.

I think it's arguable whether the better thing to do there would be to
take your approach, of extending the accepted input size to fit the
initial state + data size, or the other approach, of restricting the
size of the data[] array such that the total structure size doesn't
exceed our desired INPUT_SIZE (currently 4k).  The first bit of advice
in `perf_tips.txt` for AFL is "Keep your test cases small", so I'd be
inclined to go with the second.

But in any case, I'm in the process of reworking how the input works;
you can see the last patch posted here:

https://marc.info/?l=xen-devel&m=150774448513434

I haven't taken a close look but I think it as a side effect it will fix
this issue.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] fuzz/x86_emulate: fix bounds for input size

[Paul Semel]

On 02/23/2018 11:44 AM, George Dunlap wrote:
> Paul, thanks for reporting this!  A couple of comments...
> 
> On 02/22/2018 11:57 PM, Paul Semel wrote:
>> The minimum size for the input size was set to DATA_OFFSET + 1 which was meaning
>> that we were requesting at least one character of the data array to be filled.
>> This is not needed for the fuzzer to get working correctly.
> 
> Sure, but then the emulator will fail the first insn_fetch() callback.
> I understand there's a conceptual purity to testing such an input, but
> is it actually of practical value in finding bugs?
> 

Actually, if you really want to find bugs, you *really* want AFL to have 
control over the whole data field.

>> The maximum size for the input size was set to INPUT_SIZE, which is actually
>> the size of the data array inside the fuzz_corpus structure and so was not
>> abling user (or AFL) to fill in the whole structure. Changing to
>> sizeof(struct fuzz_corpus) correct this problem.
> 
> Good catch; actually INPUT_SIZE is a misnomer; this should really be
> DATA_SIZE.
> 
> I think it's arguable whether the better thing to do there would be to
> take your approach, of extending the accepted input size to fit the
> initial state + data size, or the other approach, of restricting the
> size of the data[] array such that the total structure size doesn't
> exceed our desired INPUT_SIZE (currently 4k).  The first bit of advice
> in `perf_tips.txt` for AFL is "Keep your test cases small", so I'd be
> inclined to go with the second.
> 

I would also go for the second, as I'm pretty sure we're not using the 
whole buffer in one run.

> But in any case, I'm in the process of reworking how the input works;
> you can see the last patch posted here:
> 
> https://marc.info/?l=xen-devel&m=150774448513434
> 

I took a look at your patch, it look really good. The only thing is that 
I think it might be more relevent to also have an option to get the 
maximum input, as I think it's the better way to find bugs as mentionned 
earlier ! 🙂

> I haven't taken a close look but I think it as a side effect it will fix
> this issue.
> 
>   -George
> 

-- 
Paul Semel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] fuzz/x86_emulate: fix bounds for input size

[Wei Liu]

On Fri, Feb 23, 2018 at 12:57:26AM +0100, Paul Semel wrote:
> The minimum size for the input size was set to DATA_OFFSET + 1 which was meaning
> that we were requesting at least one character of the data array to be filled.
> This is not needed for the fuzzer to get working correctly.

Sorry, I don't follow -- what do you expect the emulator to do if there
is no instruction to emulate?

> 
> The maximum size for the input size was set to INPUT_SIZE, which is actually
> the size of the data array inside the fuzz_corpus structure and so was not
> abling user (or AFL) to fill in the whole structure. Changing to
> sizeof(struct fuzz_corpus) correct this problem.
> 
> Signed-off-by: Paul Semel <semelpaul@gmail.com>
> ---
>  tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> index 964682aa1a..f3ce2e7e27 100644
> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> @@ -33,6 +33,7 @@ struct fuzz_corpus
>      unsigned char data[INPUT_SIZE];
>  } input;
>  #define DATA_OFFSET offsetof(struct fuzz_corpus, data)
> +#define FUZZ_CORPUS_SIZE (sizeof(struct fuzz_corpus))
>  
>  /*
>   * Internal state of the fuzzing harness.  Calculated initially from the input
> @@ -822,13 +823,13 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
>      /* Reset all global state variables */
>      memset(&input, 0, sizeof(input));
>  
> -    if ( size <= DATA_OFFSET )
> +    if ( size < DATA_OFFSET )
>      {
>          printf("Input too small\n");
>          return 1;
>      }
>  
> -    if ( size > INPUT_SIZE )
> +    if ( size > FUZZ_CORPUS_SIZE )
>      {
>          printf("Input too large\n");
>          return 1;
> @@ -859,9 +860,9 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
>  
>  unsigned int fuzz_minimal_input_size(void)
>  {
> -    BUILD_BUG_ON(DATA_OFFSET > INPUT_SIZE);
> +    BUILD_BUG_ON(DATA_OFFSET > FUZZ_CORPUS_SIZE);

Thinking more about it, this BUILD_BUG_ON is probably irrelevant
nowadays because we've opted to use struct fuzz_corpus instead of a
bunch of data structures (when the fuzzer was first implemented). I
don't think we will go back to the old model in the future so deleting
this BUILD_BUG_ON should be fine.

Wei.

>  
> -    return DATA_OFFSET + 1;
> +    return DATA_OFFSET;
>  }
>  
>  /*
> -- 
> 2.16.1
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] fuzz/x86_emulate: fix bounds for input size

[George Dunlap]

On 02/23/2018 04:30 PM, Wei Liu wrote:
> On Fri, Feb 23, 2018 at 12:57:26AM +0100, Paul Semel wrote:
>> The minimum size for the input size was set to DATA_OFFSET + 1 which was meaning
>> that we were requesting at least one character of the data array to be filled.
>> This is not needed for the fuzzer to get working correctly.
> 
> Sorry, I don't follow -- what do you expect the emulator to do if there
> is no instruction to emulate?
> 
>>
>> The maximum size for the input size was set to INPUT_SIZE, which is actually
>> the size of the data array inside the fuzz_corpus structure and so was not
>> abling user (or AFL) to fill in the whole structure. Changing to
>> sizeof(struct fuzz_corpus) correct this problem.
>>
>> Signed-off-by: Paul Semel <semelpaul@gmail.com>
>> ---
>>  tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
>> index 964682aa1a..f3ce2e7e27 100644
>> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
>> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
>> @@ -33,6 +33,7 @@ struct fuzz_corpus
>>      unsigned char data[INPUT_SIZE];
>>  } input;
>>  #define DATA_OFFSET offsetof(struct fuzz_corpus, data)
>> +#define FUZZ_CORPUS_SIZE (sizeof(struct fuzz_corpus))
>>  
>>  /*
>>   * Internal state of the fuzzing harness.  Calculated initially from the input
>> @@ -822,13 +823,13 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
>>      /* Reset all global state variables */
>>      memset(&input, 0, sizeof(input));
>>  
>> -    if ( size <= DATA_OFFSET )
>> +    if ( size < DATA_OFFSET )
>>      {
>>          printf("Input too small\n");
>>          return 1;
>>      }
>>  
>> -    if ( size > INPUT_SIZE )
>> +    if ( size > FUZZ_CORPUS_SIZE )
>>      {
>>          printf("Input too large\n");
>>          return 1;
>> @@ -859,9 +860,9 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
>>  
>>  unsigned int fuzz_minimal_input_size(void)
>>  {
>> -    BUILD_BUG_ON(DATA_OFFSET > INPUT_SIZE);
>> +    BUILD_BUG_ON(DATA_OFFSET > FUZZ_CORPUS_SIZE);
> 
> Thinking more about it, this BUILD_BUG_ON is probably irrelevant
> nowadays because we've opted to use struct fuzz_corpus instead of a
> bunch of data structures (when the fuzzer was first implemented). I
> don't think we will go back to the old model in the future so deleting
> this BUILD_BUG_ON should be fine.

We statically allocate an array of data of size INPUT_SIZE in
afl-harness.c; I think that's the purpose of the BUILD_BUG_ON(), to make
sure that that buffer is always big enough for our minimum file size.

 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] fuzz/x86_emulate: fix bounds for input size

[Wei Liu]

On Fri, Feb 23, 2018 at 04:33:18PM +0000, George Dunlap wrote:
> On 02/23/2018 04:30 PM, Wei Liu wrote:
> > On Fri, Feb 23, 2018 at 12:57:26AM +0100, Paul Semel wrote:
> >> The minimum size for the input size was set to DATA_OFFSET + 1 which was meaning
> >> that we were requesting at least one character of the data array to be filled.
> >> This is not needed for the fuzzer to get working correctly.
> > 
> > Sorry, I don't follow -- what do you expect the emulator to do if there
> > is no instruction to emulate?
> > 
> >>
> >> The maximum size for the input size was set to INPUT_SIZE, which is actually
> >> the size of the data array inside the fuzz_corpus structure and so was not
> >> abling user (or AFL) to fill in the whole structure. Changing to
> >> sizeof(struct fuzz_corpus) correct this problem.
> >>
> >> Signed-off-by: Paul Semel <semelpaul@gmail.com>
> >> ---
> >>  tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 9 +++++----
> >>  1 file changed, 5 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> >> index 964682aa1a..f3ce2e7e27 100644
> >> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> >> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> >> @@ -33,6 +33,7 @@ struct fuzz_corpus
> >>      unsigned char data[INPUT_SIZE];
> >>  } input;
> >>  #define DATA_OFFSET offsetof(struct fuzz_corpus, data)
> >> +#define FUZZ_CORPUS_SIZE (sizeof(struct fuzz_corpus))
> >>  
> >>  /*
> >>   * Internal state of the fuzzing harness.  Calculated initially from the input
> >> @@ -822,13 +823,13 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
> >>      /* Reset all global state variables */
> >>      memset(&input, 0, sizeof(input));
> >>  
> >> -    if ( size <= DATA_OFFSET )
> >> +    if ( size < DATA_OFFSET )
> >>      {
> >>          printf("Input too small\n");
> >>          return 1;
> >>      }
> >>  
> >> -    if ( size > INPUT_SIZE )
> >> +    if ( size > FUZZ_CORPUS_SIZE )
> >>      {
> >>          printf("Input too large\n");
> >>          return 1;
> >> @@ -859,9 +860,9 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
> >>  
> >>  unsigned int fuzz_minimal_input_size(void)
> >>  {
> >> -    BUILD_BUG_ON(DATA_OFFSET > INPUT_SIZE);
> >> +    BUILD_BUG_ON(DATA_OFFSET > FUZZ_CORPUS_SIZE);
> > 
> > Thinking more about it, this BUILD_BUG_ON is probably irrelevant
> > nowadays because we've opted to use struct fuzz_corpus instead of a
> > bunch of data structures (when the fuzzer was first implemented). I
> > don't think we will go back to the old model in the future so deleting
> > this BUILD_BUG_ON should be fine.
> 
> We statically allocate an array of data of size INPUT_SIZE in
> afl-harness.c; I think that's the purpose of the BUILD_BUG_ON(), to make
> sure that that buffer is always big enough for our minimum file size.

The arrangement at the moment (instruction stream input being a member
of the corpus) makes sure DATA_OFFSET will never be larger than
FUZZ_CORPUS_SIZE.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] fuzz/x86_emulate: fix bounds for input size

[Paul Semel]

On 02/23/2018 05:30 PM, Wei Liu wrote:
> On Fri, Feb 23, 2018 at 12:57:26AM +0100, Paul Semel wrote:
>> The minimum size for the input size was set to DATA_OFFSET + 1 which was meaning
>> that we were requesting at least one character of the data array to be filled.
>> This is not needed for the fuzzer to get working correctly.
> 
> Sorry, I don't follow -- what do you expect the emulator to do if there
> is no instruction to emulate?
> 

Sure, I confused myself on this one, sorry about it !

>>
>> The maximum size for the input size was set to INPUT_SIZE, which is actually
>> the size of the data array inside the fuzz_corpus structure and so was not
>> abling user (or AFL) to fill in the whole structure. Changing to
>> sizeof(struct fuzz_corpus) correct this problem.
>>
>> Signed-off-by: Paul Semel <semelpaul@gmail.com>
>> ---
>>   tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 9 +++++----
>>   1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
>> index 964682aa1a..f3ce2e7e27 100644
>> --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
>> +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
>> @@ -33,6 +33,7 @@ struct fuzz_corpus
>>       unsigned char data[INPUT_SIZE];
>>   } input;
>>   #define DATA_OFFSET offsetof(struct fuzz_corpus, data)
>> +#define FUZZ_CORPUS_SIZE (sizeof(struct fuzz_corpus))
>>   
>>   /*
>>    * Internal state of the fuzzing harness.  Calculated initially from the input
>> @@ -822,13 +823,13 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
>>       /* Reset all global state variables */
>>       memset(&input, 0, sizeof(input));
>>   
>> -    if ( size <= DATA_OFFSET )
>> +    if ( size < DATA_OFFSET )
>>       {
>>           printf("Input too small\n");
>>           return 1;
>>       }
>>   
>> -    if ( size > INPUT_SIZE )
>> +    if ( size > FUZZ_CORPUS_SIZE )
>>       {
>>           printf("Input too large\n");
>>           return 1;
>> @@ -859,9 +860,9 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
>>   
>>   unsigned int fuzz_minimal_input_size(void)
>>   {
>> -    BUILD_BUG_ON(DATA_OFFSET > INPUT_SIZE);
>> +    BUILD_BUG_ON(DATA_OFFSET > FUZZ_CORPUS_SIZE);
> 
> Thinking more about it, this BUILD_BUG_ON is probably irrelevant
> nowadays because we've opted to use struct fuzz_corpus instead of a
> bunch of data structures (when the fuzzer was first implemented). I
> don't think we will go back to the old model in the future so deleting
> this BUILD_BUG_ON should be fine.
> 

Yes, you're right 🙂

I'm going to send my last version of this patch so that you can have it !

> Wei.
> 
>>   
>> -    return DATA_OFFSET + 1;
>> +    return DATA_OFFSET;
>>   }
>>   
>>   /*
>> -- 
>> 2.16.1
>>

-- 
Paul Semel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[PATCH v2] fuzz/x86_emulate: fix bounds for input size

[Paul Semel]

The maximum size for the input size was set to INPUT_SIZE, which is actually
the size of the data array inside the fuzz_corpus structure and so was not
abling user (or AFL) to fill in the whole structure. Changing to
sizeof(struct fuzz_corpus) correct this problem.

Signed-off-by: Paul Semel <semelpaul@gmail.com>
---
 tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index 964682aa1a..0ada613f52 100644
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -33,6 +33,7 @@ struct fuzz_corpus
     unsigned char data[INPUT_SIZE];
 } input;
 #define DATA_OFFSET offsetof(struct fuzz_corpus, data)
+#define FUZZ_CORPUS_SIZE (sizeof(struct fuzz_corpus))
 
 /*
  * Internal state of the fuzzing harness.  Calculated initially from the input
@@ -828,7 +829,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
         return 1;
     }
 
-    if ( size > INPUT_SIZE )
+    if ( size > FUZZ_CORPUS_SIZE )
     {
         printf("Input too large\n");
         return 1;
@@ -859,8 +860,6 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
 
 unsigned int fuzz_minimal_input_size(void)
 {
-    BUILD_BUG_ON(DATA_OFFSET > INPUT_SIZE);
-
     return DATA_OFFSET + 1;
 }
 
-- 
2.16.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2] fuzz/x86_emulate: fix bounds for input size

[Wei Liu]

On Fri, Feb 23, 2018 at 11:48:57PM +0100, Paul Semel wrote:
> The maximum size for the input size was set to INPUT_SIZE, which is actually
> the size of the data array inside the fuzz_corpus structure and so was not
> abling user (or AFL) to fill in the whole structure. Changing to
> sizeof(struct fuzz_corpus) correct this problem.
> 
> Signed-off-by: Paul Semel <semelpaul@gmail.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2] fuzz/x86_emulate: fix bounds for input size

[George Dunlap]

On 02/23/2018 10:48 PM, Paul Semel wrote:
> The maximum size for the input size was set to INPUT_SIZE, which is actually
> the size of the data array inside the fuzz_corpus structure and so was not
> abling user (or AFL) to fill in the whole structure. Changing to
> sizeof(struct fuzz_corpus) correct this problem.
> 
> Signed-off-by: Paul Semel <semelpaul@gmail.com>

Hey Paul,

Thanks for the patch.  Looking a bit more at the code over the weekend,
I figured out what that BUILD_BUG_ON() is for -- in afl_harness.c, we
statically allocate a buffer of size INPUT_SIZE to hold the fuzz data.
The BUILD_BUG_ON() is to make sure that this buffer is always big enough
to hold the minimum input size.  And increasing the size accepted by
LLVMFuzzerTestOneInput() won't have any effect for anybody using
afl-harness, as the size passed in will never be larger than INPUT_SIZE.

Are you running afl-harness, or are you using fuzz-emul directly some
other way (e.g., through Google's fuzzing service)?

 -George


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2] fuzz/x86_emulate: fix bounds for input size

[Paul Semel]

Hey George,

On 02/27/2018 11:39 AM, George Dunlap wrote:
> Thanks for the patch. Looking a bit more at the code over the weekend,
> I figured out what that BUILD_BUG_ON() is for -- in afl_harness.c, we
> statically allocate a buffer of size INPUT_SIZE to hold the fuzz data.
> The BUILD_BUG_ON() is to make sure that this buffer is always big enough
> to hold the minimum input size.  And increasing the size accepted by
> LLVMFuzzerTestOneInput() won't have any effect for anybody using
> afl-harness, as the size passed in will never be larger than INPUT_SIZE.
>

Thanks for replying me ! Actually, I understood what this BUILD_BUG_ON() was for 
and I totally agree with you 🙂

Anyway, I am pretty sure that this check is not needed anymore for the new 
changes I made, as the condition is never reachable anymore.

> Are you running afl-harness, or are you using fuzz-emul directly some
> other way (e.g., through Google's fuzzing service)?
>

I am actually not using it, but I discovered this tool some time before, and I 
am now trying to port the idea on an other emulator project.. 🙂
Anyway, I made much changes on my own version, and if it still does interest 
you, I can share those changes with you once I'm done with my thing !

>  -George
-- 
Paul

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v2] fuzz/x86_emulate: fix bounds for input size

[Wei Liu]

On Mon, Feb 26, 2018 at 10:33:40AM +0000, Wei Liu wrote:
> On Fri, Feb 23, 2018 at 11:48:57PM +0100, Paul Semel wrote:
> > The maximum size for the input size was set to INPUT_SIZE, which is actually
> > the size of the data array inside the fuzz_corpus structure and so was not
> > abling user (or AFL) to fill in the whole structure. Changing to
> > sizeof(struct fuzz_corpus) correct this problem.
> > 
> > Signed-off-by: Paul Semel <semelpaul@gmail.com>
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>

Applied. Thanks for your patch.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel