| 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
| ||
|
Date: Mon, 12 Jul 2021 15:49:56 -0700
From: Doug Anderson <dianders@...omium.org>
To: Sumit Garg <sumit.garg@...aro.org>
Cc: kgdb-bugreport@...ts.sourceforge.net,
Daniel Thompson <daniel.thompson@...aro.org>,
Jason Wessel <jason.wessel@...driver.com>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3] kdb: Get rid of custom debug heap allocator
Hi,
On Thu, Jul 8, 2021 at 5:25 AM Sumit Garg <sumit.garg@...aro.org> wrote:
>
> @@ -233,10 +232,6 @@ extern struct task_struct *kdb_curr_task(int);
>
> @@ -52,48 +52,48 @@ int kdbgetsymval(const char *symname, kdb_symtab_t *symtab)
> }
> EXPORT_SYMBOL(kdbgetsymval);
>
> -static char *kdb_name_table[100]; /* arbitrary size */
> -
> /*
> - * kdbnearsym - Return the name of the symbol with the nearest address
nit: This is now kernel-doc, right? So start with "/**" ?
> - * less than 'addr'.
> + * kdbnearsym() - Return the name of the symbol with the nearest address
> + * less than @addr.
> + * @addr: Address to check for near symbol
> + * @symtab: Structure to receive results
> *
> - * Parameters:
> - * addr Address to check for symbol near
> - * symtab Structure to receive results
> - * Returns:
> - * 0 No sections contain this address, symtab zero filled
> - * 1 Address mapped to module/symbol/section, data in symtab
> - * Remarks:
> - * 2.6 kallsyms has a "feature" where it unpacks the name into a
> - * string. If that string is reused before the caller expects it
> - * then the caller sees its string change without warning. To
> - * avoid cluttering up the main kdb code with lots of kdb_strdup,
> - * tests and kfree calls, kdbnearsym maintains an LRU list of the
> - * last few unique strings. The list is sized large enough to
> - * hold active strings, no kdb caller of kdbnearsym makes more
> - * than ~20 later calls before using a saved value.
> + * WARNING: This function may return a pointer to a single statically
> + * allocated buffer (namebuf). kdb's unusual calling context (single
> + * threaded, all other CPUs halted) provides us sufficient locking for
> + * this to be safe. The only constraint imposed by the static buffer is
> + * that the caller must consume any previous reply prior to another call
> + * to lookup a new symbol.
> + *
> + * Note that, strictly speaking, some architectures may re-enter the kdb
> + * trap if the system turns out to be very badly damaged and this breaks
> + * the single-threaded assumption above. In these circumstances successful
> + * continuation and exit from the inner trap is unlikely to work and any
> + * user attempting this receives a prominent warning before being allowed
> + * to progress. In these circumstances we remain memory safe because
> + * namebuf[KSYM_NAME_LEN-1] will never change from '\0' although we do
> + * tolerate the possibility of garbled symbol display from the outer kdb
> + * trap.
> + *
> + * Return:
> + * * 0 - No sections contain this address, symtab zero filled
> + * * 1 - Address mapped to module/symbol/section, data in symtab
> */
> int kdbnearsym(unsigned long addr, kdb_symtab_t *symtab)
> {
> int ret = 0;
> unsigned long symbolsize = 0;
> unsigned long offset = 0;
> -#define knt1_size 128 /* must be >= kallsyms table size */
> - char *knt1 = NULL;
> + static char namebuf[KSYM_NAME_LEN];
I guess this also ends up fixing a bug too, right? My greps show that
"KSYM_NAME_LEN" is 512 and the first thing kallsyms_lookup() does it
zero out byte 511. Previously we were only allocating 128 bytes so I
guess we were writing past the end.
Assuming I understood correctly, maybe mention the bugfix in the commit text?
> @@ -102,63 +102,14 @@ int kdbnearsym(unsigned long addr, kdb_symtab_t *symtab)
> symtab->sym_end = symtab->sym_start + symbolsize;
> ret = symtab->sym_name != NULL && *(symtab->sym_name) != '\0';
>
> - if (ret) {
> - int i;
> - /* Another 2.6 kallsyms "feature". Sometimes the sym_name is
> - * set but the buffer passed into kallsyms_lookup is not used,
> - * so it contains garbage. The caller has to work out which
> - * buffer needs to be saved.
> - *
> - * What was Rusty smoking when he wrote that code?
> - */
> - if (symtab->sym_name != knt1) {
> - strncpy(knt1, symtab->sym_name, knt1_size);
> - knt1[knt1_size-1] = '\0';
> - }
> - for (i = 0; i < ARRAY_SIZE(kdb_name_table); ++i) {
> - if (kdb_name_table[i] &&
> - strcmp(kdb_name_table[i], knt1) == 0)
> - break;
> - }
> - if (i >= ARRAY_SIZE(kdb_name_table)) {
> - debug_kfree(kdb_name_table[0]);
> - memmove(kdb_name_table, kdb_name_table+1,
> - sizeof(kdb_name_table[0]) *
> - (ARRAY_SIZE(kdb_name_table)-1));
> - } else {
> - debug_kfree(knt1);
> - knt1 = kdb_name_table[i];
> - memmove(kdb_name_table+i, kdb_name_table+i+1,
> - sizeof(kdb_name_table[0]) *
> - (ARRAY_SIZE(kdb_name_table)-i-1));
> - }
> - i = ARRAY_SIZE(kdb_name_table) - 1;
> - kdb_name_table[i] = knt1;
> - symtab->sym_name = kdb_name_table[i];
> - knt1 = NULL;
I definitely had a hard time following exactly what all the cases were
handling and if they were all right, but I agree that we can kill the
old code (yay!)
> @@ -249,6 +200,7 @@ void kdb_symbol_print(unsigned long addr, const kdb_symtab_t *symtab_p,
> unsigned int punc)
> {
> kdb_symtab_t symtab, *symtab_p2;
> +
> if (symtab_p) {
> symtab_p2 = (kdb_symtab_t *)symtab_p;
> } else {
nit: unrelated whitespace change?
All comments above are nits and not terribly important, so:
Reviewed-by: Douglas Anderson <dianders@...omium.org>
Powered by blists - more mailing lists