netdev - Re: [PATCH ethtool v1 07/13] ethtool: avoid null pointer dereference

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

Date:   Wed, 7 Dec 2022 11:52:17 +0100
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Jesse Brandeburg <jesse.brandeburg@...el.com>
Cc:     e1000-patches@...ists.intel.com, netdev@...r.kernel.org
Subject: Re: [PATCH ethtool v1 07/13] ethtool: avoid null pointer dereference

On Tue, Dec 06, 2022 at 05:03:47PM -0800, Jesse Brandeburg wrote:
> '$ scan-build make' reports:
> 
> Description: Array access (from variable 'arg') results in a null
> pointer dereference
> File: /git/ethtool/netlink/parser.c
> Line: 782
> 
> Description: Dereference of null pointer (loaded from variable 'p')
> File: /git/ethtool/netlink/parser.c
> Line: 794
> 
> Both of these bugs are prevented by checking the input from netlink
> which was allowing nlctxt->argp to be NULL.

It is not input from netlink, nlctx->argp is always one of the members
in argv[] array and as argc is calculated by kernel (execve() only gets
argv, not argc), a null pointer could only be a result of a kernel bug.

If we wanted to check for null argv[] members, it would rather make
sense to do it somewhere on the global level than in each of the
handlers separately. But I'm not convinced it would help much, while we
could catch a null pointer, I'm not sure we could catch an invalid one.

> CC: Michal Kubecek <mkubecek@...e.cz>
> Fixes: 81a30f416ec7 ("netlink: add bitset command line parser handlers")
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@...el.com>

This patch is marked as "07/13" but I did not receive the rest of the
series. And even this one was not sent to netdev mailing list.

Michal

> ---
>  netlink/parser.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/netlink/parser.c b/netlink/parser.c
> index 70f451008eb4..c573a9598a9f 100644
> --- a/netlink/parser.c
> +++ b/netlink/parser.c
> @@ -874,7 +874,7 @@ int nl_parse_bitset(struct nl_context *nlctx, uint16_t type, const void *data,
>   * optionally followed by '/' and another numeric value (mask, unless no_mask
>   * is set), or a string consisting of characters corresponding to bit indices.
>   * The @data parameter points to struct char_bitset_parser_data. Generates
> - * biset nested attribute. Fails if type is zero or if @dest is not null.
> + * bitset nested attribute. Fails if type is zero or if @dest is not null.
>   */
>  int nl_parse_char_bitset(struct nl_context *nlctx, uint16_t type,
>  			 const void *data, struct nl_msg_buff *msgbuff,
> @@ -882,7 +882,7 @@ int nl_parse_char_bitset(struct nl_context *nlctx, uint16_t type,
>  {
>  	const struct char_bitset_parser_data *parser_data = data;
>  
> -	if (!type || dest) {
> +	if (!type || dest || !*nlctx->argp) {
>  		fprintf(stderr, "ethtool (%s): internal error parsing '%s'\n",
>  			nlctx->cmd, nlctx->param);
>  		return -EFAULT;
> -- 
> 2.31.1
> 

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)