On Wed, Oct 29, 2025 at 10:27:54AM +0000, Carsten Beckmann wrote:
> Hi,
> 
> I was recently debugging a protection fault in a kernel based on OpenBSD 7.7.
> Iterating over a SRP list with SRPL_FOREACH seemingly returned a corrupted
> pointer. I did trace the issue down to a bug in the SRP implementation.
> 
> srp_follow() is supposed to replace the reference held by the given srp_ref with
> a reference to the data structure that is represented by the given srp struct.
> This process is not atomic though. The old reference is given up immediately in
> srp_v() when one part of the hazard is updated with 'hzrd->sh_p = srp'.
> Afterwards the srp struct is read to get the value represented by that struct.
> 
> This causes a problem when srp_follow() is used to iterate over SRP lists. The
> current list element is only guaranteed to be kept alive while the hazard inside
> the srp_ref still points to it. As soon as that reference is given up in the
> call to srp_follow() the element may be freed. With SRP lists the srp struct
> that is read afterwards lives inside of the current element though. Accessing it
> to get a reference to the next list element can therefore be a use-after-free.
> 
> The fix is simple: Use srp_enter() to get a new reference to the next element
> while keeping the current element alive. Afterwards the old reference can safely
> be released and the hazard in the caller provided srp_ref struct can be updated
> to the hazard of the new element.

i believe this is right. i am actively trying to get rid of the SRP
code, but it doesn't hurt to fix this until i finish that.

i'll commit it soon.

thank you, this must have been a pain to figure out.

> 
> diff --git sys/kern/kern_srp.c sys/kern/kern_srp.c
> index c002fa391fd..7b702b9bde2 100644
> --- sys/kern/kern_srp.c
> +++ sys/kern/kern_srp.c
> @@ -235,7 +235,14 @@ srp_enter(struct srp_ref *sr, struct srp *srp)
>  void *
>  srp_follow(struct srp_ref *sr, struct srp *srp)
>  {
> -	return (srp_v(sr->hz, srp));
> +	struct srp_ref nsr;
> +	void *nv;
> +
> +	nv = srp_enter(&nsr, srp);
> +	srp_leave(sr);
> +	sr->hz = nsr.hz;
> +
> +	return (nv);
>  }
>  
>  void