linux-kernel - Re: [PATCH v2] lzo: fix ip overrun during compress.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 6 Dec 2018 16:03:12 +0100
From:   "Markus F.X.J. Oberhumer" <markus@...rhumer.com>
To:     Yueyi Li <liyueyi@...e.com>, "dsterba@...e.cz" <dsterba@...e.cz>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "w@....eu" <w@....eu>,
        "donb@...uritymouse.com" <donb@...uritymouse.com>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] lzo: fix ip overrun during compress.

Hi Yueyi,

yes, my LZO patch works for all cases.

The reason behind the issue in the first place is that if KASLR
includes the very last page 0xfffffffffffff000 then we do not have a
valid C "pointer to an object" anymore because of address wraparound.

Unrelated to my patch I'd argue that KASLR should *NOT* include the
very last page - indeed that might cause similar wraparound problems
in lots of code.

Eg, look at this simple clear_memory() implementation:

void clear_memory(char *p, size_t len) {
        char *end = p + len;
        while (p < end)
                *p++= 0;
}

Valid code like this will fail horribly when (p, len) is the very
last virtual page (because end will be the NULL pointer in this case).

Cheers,
Markus



On 2018-12-05 04:07, Yueyi Li wrote:
> Hi Markus,
> 
> Thanks for your review.
> 
> On 2018/12/4 18:20, Markus F.X.J. Oberhumer wrote:
>> Hi,
>>
>> I don't think that address space wraparound is legal in C, but I
>> understand that we are in kernel land and if you really want to
>> compress the last virtual page 0xfffffffffffff000 the following
>> small patch should fix that dubious case.
> 
> I guess the VA 0xfffffffffffff000 is available because KASLR be
> enabled. For this case we can see:
> 
> crash> kmem 0xfffffffffffff000
>        PAGE               PHYSICAL      MAPPING       INDEX CNT FLAGS
> ffffffbfffffffc0        1fffff000 ffffffff1655ecb9  7181fd5  2 
> 8000000000064209 locked,uptodate,owner_priv_1,writeback,reclaim,swapbacked
> 
>> This also avoids slowing down the the hot path of the compression
>> core function.
>>
>> Cheers,
>> Markus
>>
>>
>>
>> diff --git a/lib/lzo/lzo1x_compress.c b/lib/lzo/lzo1x_compress.c
>> index 236eb21167b5..959dec45f6fe 100644
>> --- a/lib/lzo/lzo1x_compress.c
>> +++ b/lib/lzo/lzo1x_compress.c
>> @@ -224,8 +224,8 @@ int lzo1x_1_compress(const unsigned char *in, size_t in_len,
>>   
>>          while (l > 20) {
>>                  size_t ll = l <= (M4_MAX_OFFSET + 1) ? l : (M4_MAX_OFFSET + 1);
>> -               uintptr_t ll_end = (uintptr_t) ip + ll;
>> -               if ((ll_end + ((t + ll) >> 5)) <= ll_end)
>> +               // check for address space wraparound
>> +               if (((uintptr_t) ip + ll + ((t + ll) >> 5)) <= (uintptr_t) ip)
>>                          break;
>>                  BUILD_BUG_ON(D_SIZE * sizeof(lzo_dict_t) > LZO1X_1_MEM_COMPRESS);
>>                  memset(wrkmem, 0, D_SIZE * sizeof(lzo_dict_t));
> I parsed panic ramdump and loaded CPU register values,  can see:
> 
> -000|lzo1x_1_do_compress(
>      |    in = 0xFFFFFFFFFFFFF000,
>      |  ?,
>      |    out = 0xFFFFFFFF2E2EE000,
>      |    out_len = 0xFFFFFF801CAA3510,
>      |  ?,
>      |    wrkmem = 0xFFFFFFFF4EBC0000)
>      |  dict = 0xFFFFFFFF4EBC0000
>      |  op = 0x1
>      |  ip = 0x9
>      |  ii = 0x9
>      |  in_end = 0x0
>      |  ip_end = 0xFFFFFFFFFFFFFFEC
>      |  m_len = 0
>      |  m_off = 1922
> -001|lzo1x_1_compress(
>      |    in = 0xFFFFFFFFFFFFF000,
>      |    in_len = 0,
>      |    out = 0xFFFFFFFF2E2EE000,
>      |    out_len = 0x00000001616FB7D0,
>      |    wrkmem = 0xFFFFFFFF4EBC0000)
>      |  ip = 0xFFFFFFFFFFFFF000
>      |  op = 0xFFFFFFFF2E2EE000
>      |  l = 4096
>      |  t = 0
>      |  ll = 4096
> 
> ll = l = in_len = 4096 in lzo1x_1_compress,  so your patch is working 
> for this panic case, but, I`m
> not sure, is it possible that in = 0xFFFFFFFFFFFFF000 and  in_len < 4096?
> 
> 
> Thanks,
> Yueyi
> 

-- 
Markus Oberhumer, <markus@...rhumer.com>, http://www.oberhumer.com/