linux-kernel - RE: [RFC][PATCH 2/2] write callback: Check if existing entry is erasable

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: <A5ED84D3BB3A384992CBB9C77DEDA4D401ABBE@USINDEM103.corp.hds.com>
Date:	Wed, 4 Jul 2012 01:54:00 +0000
From:	Seiji Aguchi <seiji.aguchi@....com>
To:	Mike Waychison <mikew@...gle.com>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Luck, Tony (tony.luck@...el.com)" <tony.luck@...el.com>,
	"Matthew Garrett (mjg@...hat.com)" <mjg@...hat.com>,
	"dzickus@...hat.com" <dzickus@...hat.com>,
	"dle-develop@...ts.sourceforge.net" 
	<dle-develop@...ts.sourceforge.net>,
	Satoru Moriya <satoru.moriya@....com>
Subject: RE: [RFC][PATCH 2/2] write callback: Check if existing entry is
 erasable

Thank you for reviewing my patch.

> > +	while (1) {
> > +		prev_why =  pstore_get_reason_str(prev_reason);
> > +		if (!strncmp(entry->var.Data, prev_why, strlen(prev_why)))
> > +			break;
> > +		prev_reason++;
> > +	}
> 
> This loop should probably be hardened to cope with bad data.  The patch description and code below show intention to overwrite
> garbage data, but this loop will never exit on garbage data.

OK. I will remove infinite loop above.

> > +	switch (prev_reason) {
> > +	case KMSG_DUMP_PANIC:
> > +	case KMSG_DUMP_EMERG:
> > +		/* Never erase panic or emergency message */
> > +		break;
> > +	case KMSG_DUMP_OOPS:
> > +	case KMSG_DUMP_RESTART:
> > +	case KMSG_DUMP_HALT:
> > +	case KMSG_DUMP_POWEROFF:
> > +		/* Can erase if new one is error message */
> > +		if (new_reason <= KMSG_DUMP_EMERG)
> > +			is_erasable = 1;
> > +		break;
> > +	default:
> > +		/* Can erase unknown message */
> > +		is_erasable = 1;
> 
> It is probably safer to actually complain here at compile time if a reason is missing.  prev_reason is an enum though, so if all cases are
> accounted for and no default is specified, gcc should complain if a new KMSG_DUMP enum value is added to the codebase without
> considering this logic here.

It makes sense.
I will remove "default" and add all cases of "KMSG_DUMP_*". 
In latest linus-tree, KMSG_DUMP_UNDEF is added.
I will check how we should treat it.


> > +		if (!can_erase_entry(entry, reason)) {
> > +			/* return without writing new entry */
> > +			spin_unlock(&efivars->lock);
> > +			*id = part;
> > +			return ret;
> > +		}
> 
> I'm not sure how comfortable I am with the code duplication here.  Was using a flag not workable?

If efi_pstore doesn't write new entry, it should return with non-zero value.
I will update my patch by changing to "return -EEXIST " or so.

Also, I will fix typo you pointed out.

Thanks,

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