Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20220128144055.207267-1-daniel.thompson@linaro.org>
In-Reply-To: <20220128144055.207267-1-daniel.thompson@linaro.org>
From:   Doug Anderson <dianders@chromium.org>
Date:   Fri, 28 Jan 2022 14:24:36 -0800
Message-ID: <CAD=FV=U7h=D+SqLU8Z12YzEjA+kDPjVZ6TxmaH8uQrg1DWT9sg@mail.gmail.com>
Subject: Re: [PATCH] kdb: Fix the putarea helper function
To:     Daniel Thompson <daniel.thompson@linaro.org>
Cc:     Jason Wessel <jason.wessel@windriver.com>,
        kgdb-bugreport@lists.sourceforge.net,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi,

On Fri, Jan 28, 2022 at 6:41 AM Daniel Thompson
<daniel.thompson@linaro.org> wrote:
>
> Currently kdb_putarea_size() uses copy_from_kernel_nofault() to write *to*
> arbitrary kernel memory. This is obviously wrong and means the memory
> modify ('mm') command is a serious risk to debugger stability: if we poke
> to a bad address we'll double-fault and lose our debug session.
>
> Fix this the (very) obvious way.
>
> Note that there are two Fixes: tags because the API was renamed and this
> patch will only trivially backport as far as the rename (and this is
> probably enough). Nevertheless Christoph's rename did not introduce this
> problem so I wanted to record that!
>
> Fixes: fe557319aa06 ("maccess: rename probe_kernel_{read,write} to copy_{from,to}_kernel_nofault")
> Fixes: 5d5314d6795f ("kdb: core for kgdb back end (1 of 2)")
> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>
> ---
>  kernel/debug/kdb/kdb_support.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/debug/kdb/kdb_support.c b/kernel/debug/kdb/kdb_support.c
> index df2bface866ef..85cb51c4a17e6 100644
> --- a/kernel/debug/kdb/kdb_support.c
> +++ b/kernel/debug/kdb/kdb_support.c
> @@ -291,7 +291,7 @@ int kdb_getarea_size(void *res, unsigned long addr, size_t size)
>   */
>  int kdb_putarea_size(unsigned long addr, void *res, size_t size)
>  {
> -       int ret = copy_from_kernel_nofault((char *)addr, (char *)res, size);
> +       int ret = copy_to_kernel_nofault((char *)addr, (char *)res, size);

Looks fine to me.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

If you wanted to further clean things up a bit, you could probably get
rid of at some of the unnecessary "char *" casts and also add a
"const", AKA:

int kdb_putarea_size(unsigned long addr, const void *res, size_t size)
{
  int ret = copy_to_kernel_nofault((void *)addr, res, size);


-Doug