Potential Use After Free in tee_se_manager_unregister_reader()

[viennadd]

Hi all,

Our code scanner Pinpoint has reported a potential use after free in the following for loop, the freed proxy still used in the for loop (var) = ((var)->field.tqe_next)),

I can see there is a critical section locked by ctx->mutex, could there are other threads that are not using this mutex object but acquiring memory from process heap in the same time (after free, but before (var) = ((var)->field.tqe_next)))? the content pointed by proxy might be overwritten by other threads then.

optee_os/core/tee/se/manager.c

Lines 75 to 91 in 24bb751

	TEE_Result tee_se_manager_unregister_reader(struct tee_se_reader *r)
	{
	struct tee_se_manager_ctx *ctx = &se_manager_ctx;
	struct tee_se_reader_proxy *proxy;
	mutex_lock(&ctx->mutex);
	TAILQ_FOREACH(proxy, &ctx->reader_proxies, link)
	{
	if (proxy->reader == r)
	TAILQ_REMOVE(&ctx->reader_proxies, proxy, link);
	free(proxy);
	}
	ctx->reader_count--;
	mutex_unlock(&ctx->mutex);
	return TEE_SUCCESS;
	}

#define	TAILQ_FOREACH(var, head, field)					\
	for ((var) = ((head)->tqh_first);				\
		(var);							\
		(var) = ((var)->field.tqe_next))

Regards,
Alex, Sourcebrella Inc.

[jenswi-linaro]

Thanks, for reporting. Using TAILQ_FOREACH_SAFE() instead of TAILQ_FOREACH() here should take care of the problem.

Would you mind supplying a pull request?

Thanks,
Jens

[viennadd]

@jenswi-linaro Sure I can do that, one more question though..

the implementation of TAILQ_FOREACH_SAFE,

#define	TAILQ_FOREACH_SAFE(var, head, field, next)			\
	for ((var) = ((head)->tqh_first);				\
		(var) != NULL && ((next) = TAILQ_NEXT(var, field), 1);	\
		(var) = (next))

if the race condition occurs, the TAILQ_NEXT inside TAILQ_FOREACH_SAFE might be still an issue ?

if proxy is freed in the loop body, the use of proxy variable in part 2 and part 3 are potential issues (memory block might be allocated by other threads and the content might not be the same) :

for (part 1; part 2; part 3)

I am thinking about an extra list to store the proxies will be freed in the future, and free those proxies after the TAILQ_FOREACH, it that OK or ?

Thank you,
Alex

[jenswi-linaro]

next here is assigned in "part 2" before the element is freed.

Moving the elements to a different list first before freeing doesn't make any difference here.
On the other hand if you're more comfortable doing so I'm probably OK with that too (I say probably since I haven't seen the code yet :-))

[viennadd]

@jenswi-linaro I thought that twice, indeed the execution sequence is "part 1" -> "part 2" -> loop body -> "part 3", I must got that wrong... I will supply a pull request in later today to replace TAILQ_FOREACH with TAILQ_FOREACH_SAFE,

Thanks,
Alex