linux-kernel - Re: [PATCH v2] bitmap: remove explicit newline handling using scnprintf format string

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: <20150429154837.d9bb4415f05418496da22b19@linux-foundation.org>
Date:	Wed, 29 Apr 2015 15:48:37 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Sudeep Holla <sudeep.holla@....com>
Cc:	linux-kernel@...r.kernel.org, Tejun Heo <tj@...nel.org>,
	Pawel Moll <Pawel.Moll@....com>,
	"Peter Zijlstra (Intel)" <peterz@...radead.org>
Subject: Re: [PATCH v2] bitmap: remove explicit newline handling using
 scnprintf format string

On Tue, 28 Apr 2015 16:36:41 +0100 Sudeep Holla <sudeep.holla@....com> wrote:

> bitmap_print_to_pagebuf uses scnprintf to copy the cpumask/list to page
> buffer. It handles the newline and trailing null character explicitly.

bitmap_print_to_pagebuf() is horrid :(  That automagic "assume caller
passed us an offset into a PAGE_SIZE area".

> It's unnecessary and also partially duplicated as scnprintf already adds
> trailing null character. The newline can be passed through format string
> to scnprintf. This patch does that simplification.
> 
> However theoritically there's one behavior difference: when the buffer
> is too small, the original code would still output '\n' at the end while
> the new code(with this patch) would just continue to print the formatted
> string. Since this function is dealing with only page buffers, it's
> highly unlikely to hit that corner case.
> 
> This patch will help in auditing the users of bitmap_print_to_pagebuf
> to verify that the buffer passed is large enough and get rid of it
> completely by replacing them with direct scnprintf()

Yes, bitmap_print_to_pagebuf() should be eliminated.  Make the callers
keep track of how much room they have in their buffer.

> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -462,19 +462,18 @@ EXPORT_SYMBOL(bitmap_parse_user);
>   * Output format is a comma-separated list of decimal numbers and
>   * ranges if list is specified or hex digits grouped into comma-separated
>   * sets of 8 digits/set. Returns the number of characters written to buf.
> + * It is assumed that the buffer passed is atleast PAGE_SIZE and easily
> + * accommodate nmaskbits.

Well kind-of.

How does this look?

--- a/lib/bitmap.c~bitmap-remove-explicit-newline-handling-using-scnprintf-format-string-fix
+++ a/lib/bitmap.c
@@ -462,8 +462,10 @@ EXPORT_SYMBOL(bitmap_parse_user);
  * Output format is a comma-separated list of decimal numbers and
  * ranges if list is specified or hex digits grouped into comma-separated
  * sets of 8 digits/set. Returns the number of characters written to buf.
- * It is assumed that the buffer passed is atleast PAGE_SIZE and easily
- * accommodate nmaskbits.
+ *
+ * It is assumed that @buf is a pointer into a PAGE_SIZE area and that
+ * sufficient storage remains at @buf to accommodate the
+ * bitmap_print_to_pagebuf() output.
  */
 int bitmap_print_to_pagebuf(bool list, char *buf, const unsigned long *maskp,
 			    int nmaskbits)

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/