refcount_t + (resend to wider audience)

refcount_t + (resend to wider audience)

[Mark Salyzyn]

(Resend to wider audience to comply with 
Documentation/networking/netdev-FAQ.txt)

Please apply/backport the following upstream feature and followup
grouped fixes patches to the stable trees (expect included in at least
3.10.y, 3.18.y, 4.4.y and 4.9.y):


GROUP 1: refcount_t feature:

f405df5de3170c00e5c54f8b7cf4766044a032ba refcount_t: Introduce a special
purpose refcount type
29dee3c03abce04cd527878ef5f9e5f91b7b83f4 locking/refcounts: Out-of-line
everything
bd174169c7a12a37b3b4aa2221f084ade010b182 locking/refcount: Add
refcount_t API kernel-doc comments
fd25d19f6b8da315332bb75936605fb45d3ea981 locking/refcount: Create
unchecked atomic_t implementation

They should merge cleanly as they are orthogonal, except for the last
one which will have a trivial merge conflict to arch/Kconfig.

Following these that add the refcount_t upstream API, the following
groups of
upsteam patches are requested because we are experiencing KASAN errors:


GROUP 2: sk_buff.users:

3889a803e1da9bd7cd10d6504bf281ee7e55dfd6 net: factor out a helper to
decrement the skb refcount
7608894e43d071ef2322a01c79522954c070ac6c net: use skb_unref() in
napi_consume_skb()
9899886d5e8ec5b343b1efe44f185a0e68dc6454 net: core: Prevent from
dereferencing null pointer when releasing SKB
633547973ffc32fd2c815639d4675e1531f0896f net: convert sk_buff.users from
atomic_t to refcount_t

They appear to merge trivially (4.9), but the last one will likely need
#include
<linux/refcount.h> added after the #include <lunux/atomic.h> in
include/linux/skbuff.h to facilitate compilation.


GROUP 3: sec_path.refcnt:

b0fcee825c0ad05057a97d1f4685e1b9e9d00c53 xfrm: Add a secpath_set helper.
55eabed60a68e918abc44f6beb64f38cc008b29d net, xfrm: convert
sec_path.refcnt from atomic_t to refcount_t

They merged with zero conflict on 4.9.


GROUP 4: sock.sk_wmem_alloc

14afee4b6092fde451ee17604e5f5c89da33e71e net: convert sock.sk_wmem_alloc
from atomic_t to refcount_t

This did not merge entirely cleanly (4.9), appears fragments for
net/ipv4/esp4.c and net/ipv6/esp6.c need to be dropped as well as minor
conflicts in two other files.


There may be more related to follow as our investigations continue, but
this is already turning into a tall order so we stop here.


Sincerely -- Mark Salyzyn

Re: refcount_t + (resend to wider audience)

[Andrew Lunn]

On Fri, Jul 28, 2017 at 10:15:23AM -0700, Mark Salyzyn wrote:
> (Resend to wider audience to comply with
> Documentation/networking/netdev-FAQ.txt)
> 
> Please apply/backport the following upstream feature and followup
> grouped fixes patches to the stable trees (expect included in at least
> 3.10.y, 3.18.y, 4.4.y and 4.9.y):

.. _stable_kernel_rules:

Hi Mark

Everything you ever wanted to know about Linux -stable releases
===============================================================

Rules on what kind of patches are accepted, and which ones are not, into the
"-stable" tree:

 - It must be obviously correct and tested.
 - It cannot be bigger than 100 lines, with context.

The first patch you list is 342 lines. The second one is 634.

Please could you read the rules and then provide some justification
for ignoring many of the rules.

    Andrew

Re: refcount_t + (resend to wider audience)

[Mark Salyzyn]

On 07/28/2017 10:41 AM, Andrew Lunn wrote:
> On Fri, Jul 28, 2017 at 10:15:23AM -0700, Mark Salyzyn wrote:
>> (Resend to wider audience to comply with
>> Documentation/networking/netdev-FAQ.txt)
>>
>> Please apply/backport the following upstream feature and followup
>> grouped fixes patches to the stable trees (expect included in at least
>> 3.10.y, 3.18.y, 4.4.y and 4.9.y):
> .. _stable_kernel_rules:
>
> Hi Mark
>
> Everything you ever wanted to know about Linux -stable releases
> ===============================================================
>
> Rules on what kind of patches are accepted, and which ones are not, into the
> "-stable" tree:
>
>   - It must be obviously correct and tested.
>   - It cannot be bigger than 100 lines, with context.
>
> The first patch you list is 342 lines. The second one is 634.
>
> Please could you read the rules and then provide some justification
> for ignoring many of the rules.
>
>      Andrew

The first four patches add a new dependent upstream API and type, 
refcount_t. New APIs will notoriously cause a large number of lines to 
be adjusted. They are complete (ToT will/should match stable), 
orthogonal, and without CONFIG_REFCOUNT_FULL completely inert in all 
places where atomic_t reference counters used, and are replaced with 
refcount_t in the followup patches that take advantage of this new type.

The first four do _nothing_ at all to the kernel as-is, but represent a 
dependency for the following changes.

The remaining patches (for the most part) take advantage of this new API 
to mostly fix, or report/warn when they can not, Use-After-Free (KASAN) 
bugs which can lead to root attack exploits. atomic_t are subject to 
unbounded attacks, refcount_t are relatively immune to unbounded 
attacks. It is admittedly not a complete fix, but greatly reduce the 
chances of the security issues. The recommendation is to turn on 
CONFIG_REFCOUNT_FULL, but that is a decision to balance between security 
and performance.

For any platform that requires the latest security updates, refcount_t 
is going to be a requirement. I urge you to overlook the first four 
patch sizes because of their status as an orthogonal type and API, 
necessary dependency for security improvements.

Sincerely -- Mark Salyzyn

Re: refcount_t + (resend to wider audience)

[David Miller]

From: Mark Salyzyn <salyzyn@android.com>
Date: Fri, 28 Jul 2017 10:15:23 -0700

> (Resend to wider audience to comply with
> Documentation/networking/netdev-FAQ.txt)
> 
> Please apply/backport the following upstream feature and followup
> grouped fixes patches to the stable trees (expect included in at least
> 3.10.y, 3.18.y, 4.4.y and 4.9.y):

I totally disagree, this is too invasive for -stable.

No way.

Re: refcount_t + (resend to wider audience)

[David Miller]

From: Mark Salyzyn <salyzyn@android.com>
Date: Fri, 28 Jul 2017 11:07:53 -0700

> On 07/28/2017 10:41 AM, Andrew Lunn wrote:
>> On Fri, Jul 28, 2017 at 10:15:23AM -0700, Mark Salyzyn wrote:
>>> (Resend to wider audience to comply with
>>> Documentation/networking/netdev-FAQ.txt)
>>>
>>> Please apply/backport the following upstream feature and followup
>>> grouped fixes patches to the stable trees (expect included in at least
>>> 3.10.y, 3.18.y, 4.4.y and 4.9.y):
>> .. _stable_kernel_rules:
>>
>> Hi Mark
>>
>> Everything you ever wanted to know about Linux -stable releases
>> ===============================================================
>>
>> Rules on what kind of patches are accepted, and which ones are not,
>> into the
>> "-stable" tree:
>>
>>   - It must be obviously correct and tested.
>>   - It cannot be bigger than 100 lines, with context.
>>
>> The first patch you list is 342 lines. The second one is 634.
>>
>> Please could you read the rules and then provide some justification
>> for ignoring many of the rules.
>>
>>      Andrew
> 
> The first four patches add a new dependent upstream API and type,
> refcount_t. New APIs will notoriously cause a large number of lines to
> be adjusted. They are complete (ToT will/should match stable),
> orthogonal, and without CONFIG_REFCOUNT_FULL completely inert in all
> places where atomic_t reference counters used, and are replaced with
> refcount_t in the followup patches that take advantage of this new
> type.
> 
> The first four do _nothing_ at all to the kernel as-is, but represent
> a dependency for the following changes.
> 
> The remaining patches (for the most part) take advantage of this new
> API to mostly fix, or report/warn when they can not, Use-After-Free
> (KASAN) bugs which can lead to root attack exploits. atomic_t are
> subject to unbounded attacks, refcount_t are relatively immune to
> unbounded attacks. It is admittedly not a complete fix, but greatly
> reduce the chances of the security issues. The recommendation is to
> turn on CONFIG_REFCOUNT_FULL, but that is a decision to balance
> between security and performance.
> 
> For any platform that requires the latest security updates, refcount_t
> is going to be a requirement. I urge you to overlook the first four
> patch sizes because of their status as an orthogonal type and API,
> necessary dependency for security improvements.

Sorry, even with this explanation this -stable require is completely
and totally inappropriate.

You guys are really pushing things way too far with this refcount_t
stuff, seriously.

NACK.

Re: refcount_t + (resend to wider audience)

[Mark Salyzyn]

On 07/28/2017 12:31 PM, David Miller wrote:
> Sorry, even with this explanation this -stable require is completely
> and totally inappropriate.
Puts me between a rock and a hard place trying to address kernel 
security issues. Should I instead file KASAN Use-After-Free reports on 
stable kernels here for analysis by those with more wisdom to help 
refine a more targeted fix?

For instance a dive on one of them did turn up 
89e357d83c06b6fac581c3ca7f0ee3ae7e67109e which stopped an unbounded 
refcounter by preventing multiple dump requests at the same time. But 
the other 4 KASAN reports I focused on this week, we were not so lucky.
> You guys are really pushing things way too far with this refcount_t
> stuff, seriously.
First round ever on this, I guess I am missing some turmoil, history or 
bad blood over refcount_t. Always fun to step on a landmine :-)
> NACK.

Please, guidance on where I can go from here.

Sincerely -- Mark Salyzyn

Re: refcount_t + (resend to wider audience)

[Andrew Lunn]

> Please, guidance on where I can go from here.

Hi Mark

Real fixes will be accepted. So check if there are any refcount fixes
which are not in stable. If so, ask for the fix to be added to stable.

A fix should be independent of the infrastructure used to find there
is an issue.

   Andrew