[lvc-project] [PATCH] net: fix potential use-after-free in ch_ipsec_xfrm_add_state() callback

[Fedor Pchelkin]

Добрый день!

На всякий случай, сообщу, что есть опция отправлять патчи в
lvc-patches at linuxtesting.org для предв. ревью перед отправкой в межд.
сообщество.  Там стараемся ловить некоторые стандартные моменты в
оформлении патчей, ну и лишний раз проблему от Свейса обсудить бывает
полезным.

Для патча в этот драйвер можно использовать префикс "ch_ipsec:" или
"cxgb4/ch_ipsec:". "net" слишком общий.

У сетевой подсистемы есть ряд своих правил в оформлении :)
https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html#netdev-faq

Для фикс-патчей целевое дерево net следует указывать в теме письма.
Например:

  [PATCH net] cxgb4/ch_ipsec: handle try_module_get() return value

On Wed, 01. Oct 14:16, Pavel Zhigulin wrote:
> In ch_ipsec_xfrm_add_state() there is not check of try_module_get
> return value. It is very unlikely, but try_module_get() could return
> false value, which could cause use-after-free error.

Не сказал бы, что описание даёт понять, при каких хотя бы теоретических
обстоятельствах функция может вернуть ошибку в данном случае.  У
мэйнтейнеров такой вопрос тоже может возникнуть.

> 
> This fix adds checking the result of try_module_get call

  Describe your changes in imperative mood, e.g. “make xyzzy do frotz”
  instead of “[This patch] makes xyzzy do frotz” or “[I] changed xyzzy to
  do frotz”, as if you are giving orders to the codebase to change its
  behaviour.
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

> +#include "asm-generic/errno-base.h"
> +#include "linux/compiler.h"

Зачем?

В этом модуле уже во многих местах возвращаются стандартные коды ошибок,
да и likely/unlikely тоже есть.

В общем случае likely/unlikely лучше использовать, когда имеются
результаты тестов-бенчмарков того, что их использование действительно
несёт пользу.  В ином случае это не несёт какой-либо явной пользы и
наоборот может ненароком даже сбить с толку компилятор.  Современные
компиляторы и CPU достаточно хорошо оптимизируют код без необоснованных
подсказок.

Впрочем, это на усмотрение Вас как автора патча и сопровождающих
подсистемы.

>  #define pr_fmt(fmt) "ch_ipsec: " fmt
> 
>  #include <linux/kernel.h>
> @@ -301,7 +303,8 @@ static int ch_ipsec_xfrm_add_state(struct net_device *dev,
>  		sa_entry->esn = 1;
>  	ch_ipsec_setkey(x, sa_entry);
>  	x->xso.offload_handle = (unsigned long)sa_entry;
> -	try_module_get(THIS_MODULE);
> +	if (unlikely(!try_module_get(THIS_MODULE)))
> +		res = -ENODEV;
>  out:
>  	return res;
>  }
> --
> 2.43.0