linux-kernel - Re: [PATCH 1/3] slab: check the return value of check_bytes_and

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]

Message-ID: <2ff52c5e-4b6b-4b3d-9047-f00967315d3e@suse.cz>
Date: Mon, 3 Jun 2024 09:46:02 +0200
From: Vlastimil Babka <vbabka@...e.cz>
To: Chengming Zhou <chengming.zhou@...ux.dev>,
 "Christoph Lameter (Ampere)" <cl@...two.org>
Cc: Pekka Enberg <penberg@...nel.org>, David Rientjes <rientjes@...gle.com>,
 Joonsoo Kim <iamjoonsoo.kim@....com>,
 Andrew Morton <akpm@...ux-foundation.org>,
 Roman Gushchin <roman.gushchin@...ux.dev>,
 Hyeonggon Yoo <42.hyeyoo@...il.com>, Feng Tang <feng.tang@...el.com>,
 linux-mm@...ck.org, linux-kernel@...r.kernel.org, zhouchengming@...edance.com
Subject: Re: [PATCH 1/3] slab: check the return value of
 check_bytes_and_report()

On 5/31/24 10:31 AM, Chengming Zhou wrote:
> On 2024/5/30 23:20, Christoph Lameter (Ampere) wrote:
>> On Tue, 28 May 2024, Chengming Zhou wrote:
>> 
>>> diff --git a/mm/slub.c b/mm/slub.c
>>> index 0809760cf789..de57512734ac 100644
>>> --- a/mm/slub.c
>>> +++ b/mm/slub.c
>>> @@ -1324,9 +1324,10 @@ static int check_object(struct kmem_cache *s, struct slab *slab,
>>>         }
>>>     } else {
>>>         if ((s->flags & SLAB_POISON) && s->object_size < s->inuse) {
>>> -            check_bytes_and_report(s, slab, p, "Alignment padding",
>>> +            if (!check_bytes_and_report(s, slab, p, "Alignment padding",
>>>                 endobject, POISON_INUSE,
>>> -                s->inuse - s->object_size);
>>> +                s->inuse - s->object_size))
>>> +                return 0;
>>>         }
>>>     }
>> 
>> This change means we will then skip the rest of the checks in check_object() such as the poison check.
> 
> Yeah, only when this padding checking failed.
> 
> Now, we always abort checking and return 0 when the first checking error happens,
> such as redzones checking above.

Yes your patch will make it consistent. But IMHO it would be better to do
all the checks without skipping, report their specific error findings in
check_bytes_and_report() but not print_trailer(). Once all checks were done,
if any found an error, print the trailer once from check_object(). Thoughts?

> Thanks.