linux-kernel - Re: [PATCH] x86/uv/time: Replace one-element array and save heap space

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:   Wed, 20 May 2020 12:01:24 -0700
From:   Joe Perches <joe@...ches.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        Darren Hart <dvhart@...radead.org>,
        Andy Shevchenko <andy@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "x86@...nel.org H. Peter Anvin" <hpa@...or.com>,
        platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
        "Gustavo A. R. Silva" <gustavo@...eddedor.com>
Subject: Re: [PATCH] x86/uv/time: Replace one-element array and save heap
 space

On Wed, 2020-05-20 at 10:19 -0700, Kees Cook wrote:
> On Mon, May 18, 2020 at 12:09:16PM -0700, Joe Perches wrote:
> > On Mon, 2020-05-18 at 14:01 -0500, Gustavo A. R. Silva wrote:
> > > The current codebase makes use of one-element arrays in the following
> > > form:
> > > 
> > > struct something {
> > >     int length;
> > >     u8 data[1];
> > > };
> > []
> > > This issue has been out there since 2009.
> > > This issue was found with the help of Coccinelle and fixed _manually_.
> > []
> > > diff --git a/arch/x86/platform/uv/uv_time.c b/arch/x86/platform/uv/uv_time.c
[]
> > > @@ -156,9 +156,8 @@ static __init int uv_rtc_allocate_timers(void)
> > >  		struct uv_rtc_timer_head *head = blade_info[bid];
> > >  
> > >  		if (!head) {
> > > -			head = kmalloc_node(sizeof(struct uv_rtc_timer_head) +
> > > -				(uv_blade_nr_possible_cpus(bid) *
> > > -					2 * sizeof(u64)),
> > > +			head = kmalloc_node(struct_size(head, cpu,
> > > +				uv_blade_nr_possible_cpus(bid)),
> > 
> > It's probably safer to use kzalloc_node here as well.
> 
> Hm, I think it's not actually needed here.

Right. Turns out it's not needed.

> All three members are
> immediately initialized and it doesn't look to ever be copied to
> userspace.

It's more that the reader doesn't have to lookup the
struct to know all the members are initialized.

It's also not a fast path so any extra time to zero
is not significant.

> > >  				GFP_KERNEL, nid);
> > >  			if (!head) {
> > >  				uv_rtc_deallocate_timers();
> 
> FWIW, I think this change is good as-is. Always nice to get back a
> little memory. ;)

Saving space, 0 to 4 bytes at a time,
depending on the heap sizes...

Using the struct_size is clearer though, so that's good too.

cheers, Joe