Re: [PATCH] tomoyo: Fix NULL pointer dereference in tomoyo_init_request_info() when domain is NULL

[Jiakai Xu]

> >> Thank you for a patch, but I don't think we need this change.
> > 
> > Thanks for your review! I understand your perspective, but I believe
> > the crash is a real NULL pointer dereference, and I'd like to explain
> > why the defensive check is warranted.
> > 
> >> TOMOYO's initial domain is &tomoyo_kernel_domain, and each thread belongs to
> >> a non-NULL domain. Therefore, tomoyo_domain() is not supposed to return NULL.
> > 
> > While tomoyo_domain() is not supposed to return NULL under normal
> > operation, there are code paths that leave s->domain_info == NULL:
> > 
> >   a) Pre-init window (security/tomoyo/tomoyo.c, lines 598-612):
> >      The task security blob is zero-allocated via kzalloc(), and
> >      security_add_hooks() at line 603 is called BEFORE
> >      s->domain_info = &tomoyo_kernel_domain at line 606. If any LSM
> >      hook fires during that window, tomoyo_domain() returns NULL.
> 
> This code is executed during early boot stage. Other LSM hooks are not
> supposed to fire.
> 
> > 
> >   b) tomoyo_task_free() (tomoyo.c, lines 533-545) explicitly sets
> >      s->domain_info = NULL after decrementing the refcount.
> 
> This code is executed when a "struct task_struct" is about to be released.
> Nobody can find this "struct task_struct". Also, this "struct task_struct"
> cannot be the current thread.
> 
> > 
> >   c) tomoyo_find_next_domain() (domain.c, lines 876-883) writes
> >      s->domain_info = NULL when the domain transition fails.
> 
> I couldn't catch, but old_domain is initialized as
> 
>   struct tomoyo_domain_info *old_domain = tomoyo_domain();
> 
> which cannot be NULL.
> 
> domain is guaranteed to be non-NULL because old_domain cannot be NULL.
> 
> 	if (!domain)
> 		domain = old_domain;
> 
> Therefore, s->domain_info is guaranteed to be non-NULL because domain cannot be NULL.
> 
> 	s->domain_info = domain;
> 
> If domain were NULL, the kernel should have already crashed at line 884.

Thank you for the thorough explanation! You are absolutely right,
and I really appreciate you taking the time to walk through each path.

> > 
> > I think adding a NULL check makes the code more robust. What do you 
> > think?
> 
> Then, this will be NULL pointer dereference.
> But fixing the location that is setting NULL is the correct approach.

I fully agree. The NULL check I proposed would only mask the symptom.
The real bug is that something corrupted the task_struct's security blob
and zeroed out domain_info before the ioctl hook fired.

Unfortunately, I don't have a reliable reproducer. The fuzzer triggered
this only once on riscv, so I can't easily track down the source of the 
corruption.

Either way, thank you again for the review. I learned a lot about
TOMOYO's domain lifecycle from your explanation.

Best regards,
Jiakai