linux-kernel - Re: [git pull] kgdb light, v5

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LFD.1.00.0802101134470.2896@woody.linux-foundation.org>
Date:	Sun, 10 Feb 2008 11:44:45 -0800 (PST)
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Jan Kiszka <jan.kiszka@....de>
cc:	Ray Lee <ray-lk@...rabbit.org>, Ingo Molnar <mingo@...e.hu>,
	Sam Ravnborg <sam@...nborg.org>, linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@....com.au>,
	Thomas Gleixner <tglx@...utronix.de>,
	Jason Wessel <jason.wessel@...driver.com>
Subject: Re: [git pull] kgdb light, v5

On Sun, 10 Feb 2008, Jan Kiszka wrote:
>
> +static int kgdb_get_mem(char *addr, unsigned char *buf, int count)
>  {
> +	if ((unsigned long)addr < TASK_SIZE)
> +		return -EFAULT;
>  
> +	return probe_kernel_read(buf, addr, count);
>  }

Ok, so this is a pretty function after all the cleanups, but I actually 
don't think that "if ((unsigned long)addr < TASK_SIZE)" is really even 
asked for.

Why not let kgdb look at user memory? I'd argue that in a lot of cases, it 
might be quite nice to do, to see what user arguments in memory are etc 
etc (think things like futexes, where user memory contents really do 
matter).

So I'd suggest getting rid of the whole "kgdb_{get|set}_mem()" functions, 
and just using "probe_kernel_{read|write}()" directly instead.

(Not that I necessarily love those names either, but whatever..)

The TASK_SIZE checks make more sense in kgdb_validate_break_address() and 
friends, where it actually does make sense to check that it's really a 
*kernel* address.

But even there, I'm not sure if the right check is to compare against 
TASK_SIZE, since kernel and user memory addresses can in theory be 
distinct (that's why we have "set_fs()" historically, and while it's no 
longer true on x86 and hasn't been in a long time, the kernel conceptually 
allows it - see my previous reply about that whole get_fs/set_fs thing in 
the definition of probe_kernel_read/write).

> +	if (count == 2 && ((long)mem & 1) == 0)
> +		err = probe_kernel_read(tmp, mem, 2);
> +	else if (count == 4 && ((long)mem & 3) == 0)
> +		err = probe_kernel_read(tmp, mem, 4);
> +	else if (count == 8 && ((long)mem & 7) == 0)
> +		err = probe_kernel_read(tmp, mem, 8);
> +	else
> +		err = probe_kernel_read(tmp, mem, count);

There's absolutely no reason to care about the alignment, since if you now 
use "probe_kernel_read()", the sane thing to do is to just do

	err = probe_kernel_read(tmp, mem, count);
	if (!err) {
		while (count > 0) {
			buf = pack_hex_byte(buf, *tmp);
			tmp++;
			count--;
	}

and you're all done. No?

		Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/