From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1755526AbcBWWrP (ORCPT <rfc822;w@1wt.eu>);
	Tue, 23 Feb 2016 17:47:15 -0500
Received: from mail-wm0-f53.google.com ([74.125.82.53]:33386 "EHLO
	mail-wm0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751943AbcBWWrO (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Tue, 23 Feb 2016 17:47:14 -0500
From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
To: Jessica Yu <jeyu@redhat.com>
Cc: Andrew Morton <akpm@linux-foundation.org>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Kees Cook <keescook@chromium.org>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] sscanf: implement basic character sets
Organization: D03
References: <1456259902-31792-1-git-send-email-jeyu@redhat.com>
X-Hashcash: 1:20:160223:keescook@chromium.org::JTS0A3jbMKrw0uvc:00000000000000000000000000000000000000001FkS
X-Hashcash: 1:20:160223:andriy.shevchenko@linux.intel.com::QP35mu16FDr8aLqZ:00000000000000000000000000002tPb
X-Hashcash: 1:20:160223:akpm@linux-foundation.org::RyLb/fidkzCZiBcF:0000000000000000000000000000000000003Xmf
X-Hashcash: 1:20:160223:jeyu@redhat.com::SHUyqcE8KlnWV1MN:005Puf
X-Hashcash: 1:20:160223:linux-kernel@vger.kernel.org::bIep1QrtPRanSLiU:0000000000000000000000000000000005WR8
Date: Tue, 23 Feb 2016 23:47:11 +0100
In-Reply-To: <1456259902-31792-1-git-send-email-jeyu@redhat.com> (Jessica Yu's
	message of "Tue, 23 Feb 2016 15:38:22 -0500")
Message-ID: <87bn77gi34.fsf@rasmusvillemoes.dk>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Feb 23 2016, Jessica Yu <jeyu@redhat.com> wrote:

> Implement basic character sets for the '%[]' conversion specifier.
>
>
>  lib/vsprintf.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 525c8e1..983358a 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -2714,6 +2714,47 @@ int vsscanf(const char *buf, const char *fmt, va_list args)
>  			num++;
>  		}
>  		continue;
> +		case '[':
> +		{
> +			char *s = (char *)va_arg(args, char *);
> +			char *set;
> +			size_t (*op)(const char *str, const char *set);
> +			size_t len = 0;
> +			bool negate = (*(fmt) == '^');
> +
> +			if (field_width == -1)
> +				field_width = SHRT_MAX;
> +
> +			op = negate ? &strcspn : &strspn;
> +			if (negate)
> +				fmt++;
> +
> +			len = strcspn(fmt, "]");
> +			/* invalid format; stop here */
> +			if (!len)
> +				return num;
> +
> +			set = kstrndup(fmt, len, GFP_KERNEL);
> +			if (!set)
> +				return num;
> +
> +			/* advance fmt past ']' */
> +			fmt += len + 1;
> +
> +			len = op(str, set);
> +			/* no matches */
> +			if (!len) {
> +				kfree(set);
> +				return num;
> +			}
> +
> +			while (len-- && field_width--)
> +				*s++ = *str++;
> +			*s = '\0';
> +			kfree(set);
> +			num++;
> +		}
> +		continue;
>  		case 'o':
>  			base = 8;
>  			break;

(1) How do we know that doing a memory allocation would be ok, and then
with GFP_KERNEL? vsnprintf can be called from just about any context, so
I don't think that would fly there. Sooner or later someone is going to
be calling sscanf with a spinlock held, methinks.

(2) I think a field width should be mandatory (so %[ should simply be
regarded as malformed - it should be %*[ or %n[ for some explicit
decimal n). That will allow the compiler or other static analyzers to do
sanity checking, and we'll probably be saved from a few buffer
overflows down the line.

It's a bit sad that the C standard doesn't include the terminating '\0'
in the field width, so one would sometimes have to write
'(int)sizeof(buf)-1', but there's not much to do about that. On that
note, it seems that your field width handling is off-by-one.

To get rid of the allocation, why not use a small bitmap? Something like

{
  char *s = (char *)va_arg(args, char *);
  DECLARE_BITMAP(map, 256) = {0};
  bool negate = false;

  /* a field width is required, and must provide room for at least a '\0' */
  if (field_width <= 0)
    return num;

  if (*fmt == '^') {
    negate = true;
    ++fmt;
  }
  for ( ; *fmt && *fmt != ']'; ++fmt)
    set_bit((u8)*fmt, map);
  if (!*fmt) // no ], so malformed input
    return num;
  ++fmt;
  if (negate) {
    bitmap_complement(map, map, 256);
    clear_bit(0, map); // this avoids testing *str != '\0' below
  }

  if (!test_bit((u8)*str, map)) // match must be non-empty
    return num;
  while (test_bit((u8)*str, map) && --field_width) {
    *s++ = *str++;
  }
  *s = '\0';
  ++num;
}

Rasmus