linux-kernel - Re: [PATCH] kdb: Fix the putarea helper function

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: <CAD=FV=U7h=D+SqLU8Z12YzEjA+kDPjVZ6TxmaH8uQrg1DWT9sg@mail.gmail.com>
Date:   Fri, 28 Jan 2022 14:24:36 -0800
From:   Doug Anderson <dianders@...omium.org>
To:     Daniel Thompson <daniel.thompson@...aro.org>
Cc:     Jason Wessel <jason.wessel@...driver.com>,
        kgdb-bugreport@...ts.sourceforge.net,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] kdb: Fix the putarea helper function

Hi,

On Fri, Jan 28, 2022 at 6:41 AM Daniel Thompson
<daniel.thompson@...aro.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@...aro.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@...omium.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