[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20221208062312.2emtsvurflldumsr@lion.mk-sys.cz>
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