Bug 206755 - Use of initialised stack variables in tdfx_query_update
Summary: Use of initialised stack variables in tdfx_query_update
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Mahdi Mokhtari
URL: https://reviews.freebsd.org/D5132
Keywords: patch
Depends on:
Blocks:
 
Reported: 2016-01-30 09:53 UTC by CTurt
Modified: 2016-10-14 19:17 UTC (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description CTurt 2016-01-30 09:53:38 UTC
`tdfx_query_update` in `sys/dev/tdfx/tdfx_pci.c` doesn't check the result of `copyin` calls:

static int
tdfx_query_update(u_int cmd, struct tdfx_pio_data *piod)
{
	/* XXX Comment this later, after careful inspection and spring cleaning :) */
	/* Return vals */
	u_int8_t  ret_byte;
	u_int16_t ret_word;
	u_int32_t ret_dword;
	
	...
	
	switch (piod->size) {
		case 1:
			copyin(piod->value, &ret_byte, 1);
			preval = ret_byte << (8 * (piod->port & 0x3));
			mask = 0xff << (8 * (piod->port & 0x3));
			break;
		case 2:
			copyin(piod->value, &ret_word, 2);
			preval = ret_word << (8 * (piod->port & 0x3));
			mask = 0xffff << (8 * (piod->port & 0x3));
			break;
		case 4:
			copyin(piod->value, &ret_dword, 4);
			preval = ret_dword;
			mask = ~0;
			break;
		default:
			return -EINVAL;
	}
	/* Finally, combine the values and write it to the port */
	retval = (retval & ~mask) | preval;
	pci_write_config(tdfx_info->dev, piod->port & ~3, retval, 4);

If the user supplies a bad pointer, so that the `copyin` calls fail, `pci_write_config` will be passed an uninitialised stack value.
Comment 2 CTurt 2016-01-30 10:06:12 UTC
Missed one of the `copyin` calls in my original patch, an additional one is needed as well: https://github.com/HardenedBSD/hardenedBSD-playground/commit/ccf98fe9312539aca1154a9462d611d8fdc4f5fa.patch
Comment 3 Shawn Webb 2016-03-01 20:59:43 UTC
Any movement on this?
Comment 4 Mahdi Mokhtari freebsd_committer 2016-03-02 11:09:11 UTC
(In reply to Shawn Webb from comment #3)
patch submitted and committed AFAIK :D
Comment 5 Shawn Webb 2016-03-02 12:59:41 UTC
(In reply to Mahdi Mokhtari from comment #4)

Seems like it's still uncommitted. The patch on Phabricator is still marked as open.
Comment 6 Mahdi Mokhtari freebsd_committer 2016-03-02 13:19:04 UTC
(In reply to Shawn Webb from comment #5)
@Shawn: i see it's committed on source.
Try to update your local src tree (if you have one).
maybe we should close that review on Phabricator, or contact committer to do that.