linux-kernel - Re: [PATCH 1/2] topology: use bin_attribute to avoid buff overflow

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, 2 Jun 2021 17:00:16 +0800
From:   "tiantao (H)" <tiantao6@...wei.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
CC:     Greg KH <gregkh@...uxfoundation.org>,
        Tian Tao <tiantao6@...ilicon.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        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


在 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 ?


>
>>>> +       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;
>>>> +}
>
> --
> With Best Regards,
> Andy Shevchenko
> .
>