[kernel-hardening] Re: Conversion from atomic_t to refcount_t: summary of issues

[Peter Zijlstra <peterz@infradead.org>]

On Fri, Dec 02, 2016 at 05:44:34PM +0200, Liljestrand Hans wrote:
> 
> Then there's at least include/net/ip_vs.h that does unchecked decs and
> instead has this dedicated free function that checks for negative values
> (so with unsigned refcount it is broken anyway, guess we could do a
> conditional dec with a _read, but then its no longer atomic):
> 
> http://lxr.free-electrons.com/source/include/net/ip_vs.h#L1424
> 
>  static inline void ip_vs_dest_put_and_free(struct ip_vs_dest *dest) 
>  {
>  	if (atomic_dec_return(&dest->refcnt) < 0)
>  		kfree(dest);
>  }

This looks like one that uses -1 to free, so doing a +1 on the entire
scheme would restore 'sanity', but that's fairly thick code and I
couldn't say for sure.

> Then there's cases that check for the first increment, like here (maybe
> something like inc_and_one could allow these without too much leeway?):
> 
> http://lxr.free-electrons.com/source/drivers/tty/serial/zs.c#L764
> 
>  irq_guard = atomic_add_return(1, &scc->irq_guard);
>  	if (irq_guard == 1) {
> 
> http://lxr.free-electrons.com/source/drivers/usb/gadget/function/f_fs.c#L1497
> 
>  if (atomic_add_return(1, &ffs->opened) == 1 &&
>  	ffs->state == FFS_DEACTIVATED) {
> 
> 
> And finally some cases with other uses/values:
> 
> http://lxr.free-electrons.com/source/drivers/staging/lustre/lustre/ptlrpc/client.c#L3081
> 
>  if (atomic_inc_return(&req->rq_refcount) == 2)

Greg already went through these, they're not proper refcounts.


> http://lxr.free-electrons.com/source/kernel/bpf/syscall.c#L231
> 
>  if (atomic_inc_return(&map->refcnt) > BPF_MAX_REFCNT) {

I think this one already got discussed, its a custom refcount limit
scheme (with holes in).

All in all I'm not inclined to add {add,sub.inc,dec}_return() to
refcount, as previously stated, they don't make sense.