Date:   Thu, 8 Dec 2022 07:23:12 +0100
From:   Michal Kubecek <mkubecek@...e.cz>
To:     Jesse Brandeburg <jesse.brandeburg@...el.com>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH ethtool v2 07/13] ethtool: avoid null pointer dereference

On Wed, Dec 07, 2022 at 05:11:16PM -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 in
> nl_parse_char_bitset(), which is called from nl_sset() via the kernel
> callback, specifically for the parsing of the wake-on-lan options (-s
> wol). None of the other functions in this file seem to have the issue of
> deferencing data without checking for validity first. This could
> "technically" allow nlctxt->argp to be NULL, and scan-build is limited
> in it's ability to parse for bugs only at file scope in this case.
> This particular bug should be unlikely to happen because the kernel
> builds/parses the netlink structure before handing it to the

Again: this has nothing to do with netlink, this is command line parser,
nlctx->argp is a member of argv[] array. And as execve() (which is the
only syscall in the exec* family, the rest are wrappers) does not pass
argc, only argv[], argc is actually determined by kernel so for it to be
actually null, you would need a serious bug in kernel first.

Even if we want to be safe against buggy kernel passing garbage as
command line arguments, I still believe we should do that earlier, in
the general code, not deep in a specific helper function. Also, you only
check for null but that does not catch an invalid pointer in argv[]
which, unlike a null pointer, could do an actual harm. And I don't see
how that could be checked, I'm afraid we have to trust kernel.

Michal

> application. However in the interests of being able to run this
> scan-build tool regularly, I'm still sending the initial version of this
> patch as I tried several other ways to fix the bug with an earlier check
> for NULL in nl_sset, but it won't prevent the scan-build error due to
> the file scope problem.
> 
> CC: Michal Kubecek <mkubecek@...e.cz>
> Fixes: 81a30f416ec7 ("netlink: add bitset command line parser handlers")
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@...el.com>
> ---
> v2: updated commit message with more nuance after feedback from ethtool
> maintainer. I'd be open to fixing this a different way but this seemed
> the most straight-forward with the smallest amount of code changed.
> v1: original version
> ---
>  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)

Powered by blists - more mailing lists