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]
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/

Powered by blists - more mailing lists