linux-kernel - Re: [PATCH] lib/parser.c: avoid overflow in match

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] [day] [month] [year] [list]

Date:	Fri, 10 Aug 2012 10:26:12 -0700
From:	Randy Dunlap <rdunlap@...otime.net>
To:	Alex Elder <elder@...tank.com>
CC:	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH]  lib/parser.c: avoid overflow in match_number()

On 08/09/2012 01:03 PM, Alex Elder wrote:

> The result of converting an integer value to another signed integer
> type that's unable to represent the original value is implementation
> defined.  (See notes in section 6.3.1.3 of the C standard.)
> 
> In match_number(), the result of simple_strtol() (which returns type
> long) is assigned to a value of type int.
> 
> Instead, handle the result of simple_strtol() in a well-defined way,
> and return -ERANGE if the result won't fit in the int variable used
> to hold the parsed result.
> 
> No current callers pay attention to the particular error value
> returned, so this additional return code shouldn't do any harm.
> 
> Signed-off-by: Alex Elder <elder@...tank.com>


Makes sense to me, but I wonder who will merge it.

I'm Cc-ing a couple of possibilities.

> ---
>  lib/parser.c |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> Index: b/lib/parser.c
> ===================================================================
> --- a/lib/parser.c
> +++ b/lib/parser.c
> @@ -122,13 +122,14 @@ int match_token(char *s, const match_tab
>   *
>   * Description: Given a &substring_t and a base, attempts to parse the substring
>   * as a number in that base. On success, sets @result to the integer represented
> - * by the string and returns 0. Returns either -ENOMEM or -EINVAL on failure.
> + * by the string and returns 0. Returns -ENOMEM, -EINVAL, or -ERANGE on failure.
>   */
>  static int match_number(substring_t *s, int *result, int base)
>  {
>      char *endp;
>      char *buf;
>      int ret;
> +    long val;
>      size_t len = s->to - s->from;
> 
>      buf = kmalloc(len + 1, GFP_KERNEL);
> @@ -136,10 +137,15 @@ static int match_number(substring_t *s,
>          return -ENOMEM;
>      memcpy(buf, s->from, len);
>      buf[len] = '\0';
> -    *result = simple_strtol(buf, &endp, base);
> +
>      ret = 0;
> +    val = simple_strtol(buf, &endp, base);
>      if (endp == buf)
>          ret = -EINVAL;
> +    else if (val < (long) INT_MIN || val > (long) INT_MAX)
> +        ret = -ERANGE;
> +    else
> +        *result = (int) val;
>      kfree(buf);
>      return ret;
>  }
> -- 


-- 
~Randy
--
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/