[PATCH] kdb: Fix the putarea helper function

[PATCH] kdb: Fix the putarea helper function

[Daniel Thompson]

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);
 	if (ret) {
 		if (!KDB_STATE(SUPPRESS)) {
 			kdb_func_printf("Bad address 0x%lx\n", addr);

base-commit: e783362eb54cd99b2cac8b3a9aeac942e6f6ac07
--
2.34.1

Re: [PATCH] kdb: Fix the putarea helper function

[Doug Anderson]

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