linux-kernel - Re: [PATCH] fix: add safety check for OPTION

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]

Message-ID: <Z1D0LNfWcnPkWC3_@google.com>
Date: Wed, 4 Dec 2024 16:30:36 -0800
From: Namhyung Kim <namhyung@...nel.org>
To: Atharva Tiwari <evepolonium@...il.com>
Cc: atharvatiwari1101@...look.in, Michael Ellerman <mpe@...erman.id.au>,
	Nicholas Piggin <npiggin@...il.com>,
	Christophe Leroy <christophe.leroy@...roup.eu>,
	Naveen N Rao <naveen@...nel.org>,
	Madhavan Srinivasan <maddy@...ux.ibm.com>,
	Arnaldo Carvalho de Melo <acme@...hat.com>,
	Ian Rogers <irogers@...gle.com>,
	Aditya Gupta <adityag@...ux.ibm.com>, Jiri Olsa <jolsa@...nel.org>,
	Eder Zulian <ezulian@...hat.com>, linuxppc-dev@...ts.ozlabs.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fix: add safety check for OPTION_END and refactor code
 for consistency

Hello,

On Fri, Nov 29, 2024 at 04:13:53PM +0530, Atharva Tiwari wrote:
> - Added a null check for 'o' before copying the last OPTION_END in options__order function to prevent potential uninitialized usage.
> - Refactored the parse_long_opt function for improved readability by aligning function signature.
> - Minor formatting fix to ensure consistency in the codebase.
> - Updated the wrapper script for pseries architecture to handle 'notext' option in a more reliable way.
> 
> Signed-off-by: Atharva Tiwari <atharvatiwari1101@...look.in>
> ---
>  arch/powerpc/boot/wrapper        |  1 +
>  tools/lib/subcmd/parse-options.c | 10 +++++-----
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/powerpc/boot/wrapper b/arch/powerpc/boot/wrapper
> index 1db60fe13802..d25ad8c622f4 100755
> --- a/arch/powerpc/boot/wrapper
> +++ b/arch/powerpc/boot/wrapper
> @@ -267,6 +267,7 @@ pseries)
>      link_address='0x4000000'
>      if [ "$format" != "elf32ppc" ]; then
>  	link_address=
> +	notext='-z notext'

This is a completely differnt change and should be sent to powerpc devs
separately.


>  	pie=-pie
>      fi
>      make_space=n
> diff --git a/tools/lib/subcmd/parse-options.c b/tools/lib/subcmd/parse-options.c
> index 555d617c1f50..f85b642bc9aa 100644
> --- a/tools/lib/subcmd/parse-options.c
> +++ b/tools/lib/subcmd/parse-options.c
> @@ -360,8 +360,7 @@ static int parse_short_opt(struct parse_opt_ctx_t *p, const struct option *optio
>  	return -2;
>  }
>  
> -static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg,
> -                          const struct option *options)
> +static int parse_long_opt(struct parse_opt_ctx_t *p, const char *arg, const struct option *options)

Please don't change this unnecessarily.


>  {
>  	const char *arg_end = strchr(arg, '=');
>  	const struct option *abbrev_option = NULL, *ambiguous_option = NULL;
> @@ -828,9 +827,10 @@ static struct option *options__order(const struct option *opts)
>  
>  		nr_parent = nr_opts;
>  	}
> -	/* copy the last OPTION_END */
> -	memcpy(&ordered[nr_opts], o, sizeof(*o));
> -
> +	/* Check whether o is  valid before using it to copy the last OPTION_END. */
> +	if (o && o->type == OPTION_END) {
> +		memcpy(&ordered[nr_opts], o, sizeof(*o));
> +	}

Do you any real problems with this?  I think the existing code assumes
the last entry should be OPTION_END.  Otherwise it won't work well.

Thanks,
Namhyung


>  	/* sort each option group individually */
>  	for (opt = group = ordered; opt->type != OPTION_END; opt++) {
>  		if (opt->type == OPTION_GROUP) {
> -- 
> 2.43.-1
>