| 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
| ||
|
Message-ID: <547f8273faa146699dd50173b33f03e2@hisilicon.com>
Date: Wed, 2 Jun 2021 09:20:45 +0000
From: "Song Bao Hua (Barry Song)" <song.bao.hua@...ilicon.com>
To: Greg KH <gregkh@...uxfoundation.org>,
"tiantao (H)" <tiantao6@...ilicon.com>
CC: Andy Shevchenko <andy.shevchenko@...il.com>,
"tiantao (H)" <tiantao6@...ilicon.com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
"Rafael J. Wysocki" <rafael@...nel.org>,
Jonathan Cameron <jonathan.cameron@...wei.com>
Subject: RE: [PATCH 1/2] topology: use bin_attribute to avoid buff overflow
> -----Original Message-----
> From: Greg KH [mailto:gregkh@...uxfoundation.org]
> Sent: Wednesday, June 2, 2021 9:06 PM
> To: tiantao (H) <tiantao6@...ilicon.com>
> Cc: Andy Shevchenko <andy.shevchenko@...il.com>; tiantao (H)
> <tiantao6@...ilicon.com>; Linux Kernel Mailing List
> <linux-kernel@...r.kernel.org>; Andrew Morton <akpm@...ux-foundation.org>;
> Song Bao Hua (Barry Song) <song.bao.hua@...ilicon.com>; Andy Shevchenko
> <andriy.shevchenko@...ux.intel.com>; Rafael J. Wysocki <rafael@...nel.org>;
> Jonathan Cameron <jonathan.cameron@...wei.com>
> Subject: Re: [PATCH 1/2] topology: use bin_attribute to avoid buff overflow
>
> On Wed, Jun 02, 2021 at 05:00:16PM +0800, tiantao (H) wrote:
> >
> > 在 2021/6/2 16:48, Andy Shevchenko 写道:
> > > On Wed, Jun 2, 2021 at 9:45 AM tiantao (H) <tiantao6@...wei.com> wrote:
> > > > 在 2021/6/2 14:18, Greg KH 写道:
> > > > > On Wed, Jun 02, 2021 at 02:14:49PM +0800, tiantao (H) wrote:
> > > > > > 在 2021/6/1 12:58, Greg KH 写道:
> > > > > > > On Tue, Jun 01, 2021 at 10:56:49AM +0800, Tian Tao wrote:
> > > ...
> > >
> > > > > > > > /**
> > > > > > > > + * bitmap_print_to_buf - convert bitmap to list or hex format
> ASCII string
> > > > > > > > + * @list: indicates whether the bitmap must be list
> > > > > > > > + * @buf: page aligned buffer into which string is placed
> > > > > > > > + * @maskp: pointer to bitmap to convert
> > > > > > > > + * @nmaskbits: size of bitmap, in bits
> > > > > > > > + * @off: offset in buf
> > > > > > > > + * @count: count that already output
> > > > > > > > + *
> > > > > > > > + * the role of bitmap_print_to_buf and bitmap_print_to_pagebuf
> is
> > > > > > > > + * the same, the difference is that the second parameter of
> > > > > > > > + * bitmap_print_to_buf can be more than one pagesize.
> > > > > > > > + */
> > > > > > > > +int bitmap_print_to_buf(bool list, char *buf, const unsigned
> long *maskp,
> > > > > > > > + int nmaskbits, loff_t off, size_t count)
> > > > > > > > +{
> > > > > > > > + int len, size;
> > > > > > > > + void *data;
> > > > > > > > + char *fmt = list ? "%*pbl\n" : "%*pb\n";
> > > > > > > > +
> > > > > > > > + len = snprintf(NULL, 0, fmt, nmaskbits, maskp);
> > > > > > > > +
> > > > > > > > + data = kvmalloc(len+1, GFP_KERNEL);
> > > > > > > > + if (!data)
> > > > > > > > + return -ENOMEM;
> > > > > > > > +
> > > > > > > > + size = scnprintf(data, len+1, fmt, nmaskbits, maskp);
> > > > > > > > + size = memory_read_from_buffer(buf, count, &off, data, size);
> > > > > > > > + kvfree(data);
> > > > > > > > +
> > > > > > > > + return size;
> > > > > > > Why is this so different from bitmap_print_to_pagebuf()? Can't you
> just
> > > > > > > use this function as the "real" function and then change
> > > > > > > bitmap_print_to_pagebuf() to call it with a size of PAGE_SIZE?
> > > > > > Do you mean do following change, is that correct? :-)
> > > > > Maybe, it is whitespace corrupted, and it still feels like this function
> > > > > is much bigger than it needs to be given the function it is replacing
> is
> > > > > only a simple sprintf() call.
> > > > >
> > > > > > +int bitmap_print_to_buf(bool list, char *buf, const unsigned long
> *maskp,
> > > > > > + int nmaskbits, loff_t off, size_t count)
> > > > > > +{
> > > > > > + int len, size;
> > > > > > + void *data;
> > > > > > + const char *fmt = list ? "%*pbl\n" : "%*pb\n";
> > > > > > +
> > > > > > + if (off == LLONG_MAX && count == PAGE_SIZE - offset_in_page(buf))
> > > > > > + return scnprintf(buf, count, fmt, nmaskbits, maskp);
> > > > > > +
> > > > > > + len = snprintf(NULL, 0, fmt, nmaskbits, maskp);
> > > > > > +
> > > > > > + data = kvmalloc(len+1, GFP_KERNEL);
> > > > > Why do you need to allocate more memory? And why kvmalloc()?
> > > > Because the memory here will exceed a pagesize and we don't know the
> > > > exact size, we have to call
> > > >
> > > > snprintf first to get the actual size. kvmalloc() is used because when
> > > > physical memory is tight, kmalloc
> > > >
> > > > may fail, but vmalloc will succeed. It is not so bad that the memory is
> > > > not requested here.
> > > To me it sounds like the function is overengineered / lacks thought
> > > through / optimization.
> > > Can you provide a few examples that require the above algorithm?
> >
> > so you think we should use kmalloc instead of kvmalloc ?
>
> What size bitmap would trigger a vmalloc() call to be forced here?
>
According to kvmalloc_node(), only if size is larger than PAGE_SIZE,
kvmalloc will move to vmalloc if kmalloc fails to get memory. Otherwise,
it will return error.
void *kvmalloc_node(size_t size, gfp_t flags, int node)
{
gfp_t kmalloc_flags = flags;
void *ret;
...
ret = kmalloc_node(size, kmalloc_flags, node);
/*
* It doesn't really make sense to fallback to vmalloc for sub page
* requests
*/
if (ret || size <= PAGE_SIZE)
return ret;
return __vmalloc_node(size, 1, flags, node,
__builtin_return_address(0));
}
For bitmap, it is clear a large NR_CPUS can trigger vmalloc:
Code copy-paste from drivers/base/node.c:
/* 2008/04/07: buf currently PAGE_SIZE, need 9 chars per 32 bits. */
BUILD_BUG_ON((NR_CPUS/32 * 9) > (PAGE_SIZE-1));
But for list, it would be much more tricky. As a list could be as simple
as:
0-2047
It could also be as complex as:
0,1,3,5,7,9,11,13,.....,2045,2047
It totally depends on how the bitmap is like.
That's why tiantao's code is detecting size before malloc.
> thanks,
>
> greg k-h
Thanks
Barry
Powered by blists - more mailing lists