Re: [Kgdb-bugreport] [PATCH] kgdb: Read buffer overflow

[Andrew M. <ak...@li...>]

On Tue, 28 Jul 2009 14:23:16 -0500
Jason Wessel <jas...@wi...> wrote:

> 
> 
> 
> Roel Kluin wrote:
> > if atomic_read(kgdb_active) can ever return -1 (kgdb_active is
> initialized to -1),
> > tid also becomes -1, leading to reads from kgdb_info[-1] on the
> following lines.
> >
> 
> 
> 
> Certainly I can see how some kind of source analyzer could find holes
> in logic, but it does not seem to take in to account how the code
> actually works.
> 
> It is important to point out that it is impossible for the
> atomic_read(&kgdb_active) to return anything outside of 0..[NR_CPUS -
> 1], in the context of how this function is used.
> 
> It is only safe to call getthread(), which is a private kgdb core
> function, from a context where kgdb_active is set.  The kgdb_active
> value represents is the cpu number of which cpu is the kgdb master
> when a debugger is talking to kgdb.
> 
> I am inclined to reject the patch or change it to a out right panic /
> reboot / death because if kgdb_active did change there, the kgdb core's
> ability to restore the system to a run state == NOT POSSIBLE.
> 
> Does your tool that you used for this analysis consider the function
> scoping?

I don't know, but...

Parfait found a large number of real kernel bugs, so we hope that
people will run it regularly.  To help those people in their project it
would be good to a) report false positives to the parfait developers
and b) rework the kernel code to make the false positives go away, so
people don't keep reporting and "fixing" them.

> The only place that can change the kgdb_active value is in
> kgdb_handle_exception().  All the code that references the function
> you wanted to change is called from functions with in
> gdb_serial_stub().  The value of kgdb_active is set to a valid value
> prior to calling gdb_serial_stub(), and then set back to -1 after it
> returns in order to restore the system run state.
> 
> Jason.
>