Re: [PATCH] grep: fall back to interpreter mode if JIT fails

[Mathias Krause <minipli@grsecurity.net>]

[-- Attachment #1.1: Type: text/plain, Size: 3722 bytes --]

Am 16.12.22 um 17:12 schrieb Ævar Arnfjörð Bjarmason:
> 
> On Fri, Dec 16 2022, Mathias Krause wrote:
> 
>> From: Carlo Marcelo Arenas Belón <carenas@gmail.com>
>>
>> Under Linux systems with SELinux's 'deny_execmem' or PaX's MPROTECT
>> enabled, the allocation of PCRE2's JIT rwx memory may be prohibited,
>> making pcre2_jit_compile() fail with PCRE2_ERROR_NOMEMORY (-48):
>>
>>   [user@fedora git]$ git grep -c PCRE2_JIT
>>   grep.c:1
>>
>>   [user@fedora git]$ # Enable SELinux's W^X policy
>>   [user@fedora git]$ sudo semanage boolean -m -1 deny_execmem
>>
>>   [user@fedora git]$ # JIT memory allocation fails, breaking 'git grep'
>>   [user@fedora git]$ git grep -c PCRE2_JIT
>>   fatal: Couldn't JIT the PCRE2 pattern 'PCRE2_JIT', got '-48'
>>
>> Instead of failing hard in this case and making 'git grep' unusable on
>> such systems, simply fall back to interpreter mode, leading to a much
>> better user experience.
>>
>> Such a change was already proposed 4 years ago [1] but wasn't merged for
>> unknown reasons.
> 
> Yeah, it's unfortunate that it fell between the cracks, and it's good to
> have such a fallback mechanism.

I fully agree, obviously!

>> 1. https://lore.kernel.org/r/20181209230024.43444-3-carenas@gmail.com
>>
>> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
>> Signed-off-by: Mathias Krause <minipli@grsecurity.net>	# tweaked changelog, added comment
>> ---
>>  grep.c | 17 +++++++++++++++--
>>  1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/grep.c b/grep.c
>> index 06eed694936c..f2ada528b21d 100644
>> --- a/grep.c
>> +++ b/grep.c
>> @@ -317,8 +317,21 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
>>  	pcre2_config(PCRE2_CONFIG_JIT, &p->pcre2_jit_on);
>>  	if (p->pcre2_jit_on) {
>>  		jitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE);
>> -		if (jitret)
>> -			die("Couldn't JIT the PCRE2 pattern '%s', got '%d'\n", p->pattern, jitret);
>> +		if (jitret) {
>> +			/*
>> +			 * Even though pcre2_config(PCRE2_CONFIG_JIT, ...)
>> +			 * indicated JIT support, the library might still
>> +			 * fail to generate JIT code for various reasons,
>> +			 * e.g. when SELinux's 'deny_execmem' or PaX's
>> +			 * MPROTECT prevent creating W|X memory mappings.
>> +			 *
>> +			 * Instead of faling hard, fall back to interpreter
>> +			 * mode, just as if the pattern was prefixed with
>> +			 * '(*NO_JIT)'.
>> +			 */
>> +			p->pcre2_jit_on = 0;
>> +			return;
> 
> From my reading of the docs it returns two different codes:
> PCRE2_ERROR_JIT_BADOPTION or PCRE2_ERROR_NOMEMORY.
> 
> This change will start treating both the same, but we only want to allow
> the latter, surely?
> 
> So shouldn't this be e.g.:
> 
> 	jitret = pcre2_jit_compile(...);
> 	if (jitret == PCRE2_ERROR_NOMEMORY) {
> 		/* code you added here */
> 	} else if (jitret) {
> 		BUG(...);
> 	}

I'm fine with it either way. In fact, just testing for the "JIT memory
cannot be allocated" error might be more on point and could detect wrong
API usage by the means of an error. However, from a user's point of view
a fall back to interpreter mode might still be desired in this case, as
a failing 'git grep' is simply not acceptable, IMHO.

> I put a BUG() there, we could keep the die(), but
> PCRE2_ERROR_JIT_BADOPTION is really more appropriate as a BUG(), and if
> it starts returning any other codes our use of the API is also in some
> unknown state, so we should also BUG() out.

Valid points. I'll give others some more time to churn in and if there
are no strong objections, I'll change it like you suggest.

Thanks,
Mathias

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 665 bytes --]