Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   THOBY Simon <Simon.THOBY@viveris.fr>
To:     Mimi Zohar <zohar@linux.ibm.com>, liqiong <liqiong@nfschina.com>
CC:     "dmitry.kasatkin@gmail.com" <dmitry.kasatkin@gmail.com>,
        "jmorris@namei.org" <jmorris@namei.org>,
        "serge@hallyn.com" <serge@hallyn.com>,
        "linux-integrity@vger.kernel.org" <linux-integrity@vger.kernel.org>,
        "linux-security-module@vger.kernel.org" 
        <linux-security-module@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] ima: fix infinite loop within "ima_match_policy"
 function.
Thread-Topic: [PATCH] ima: fix infinite loop within "ima_match_policy"
 function.
Thread-Index: AQHXlPnZCsK5CNi7NkWxUIAyoKg2DKt8LpyAgAA0rYCAAChigIAD4XiAgABUhACAAAJBAIAAPl8AgAABMIA=
Date:   Mon, 23 Aug 2021 12:02:13 +0000
Message-ID: <a4302c76-5865-a8f5-e754-c5dd04030533@viveris.fr>
References: <20210819101529.28001-1-liqiong@nfschina.com>
 <8d17f252-4a93-f430-3f25-e75556ab01e8@viveris.fr>
 <d385686b-ffa5-5794-2cf2-b87f2a471e78@nfschina.com>
 <1f631c3d-5dce-e477-bfb3-05aa38836442@viveris.fr>
 <96037695de6125c701889c168550def278adfd4b.camel@linux.ibm.com>
 <f9798484-7090-0ddf-50a6-7c7c5bf0606c@nfschina.com>
 <fee498ec-087c-b52d-102c-d29d98f9b794@nfschina.com>
 <cf715a40-b255-c688-578c-7f8bcd004ee3@viveris.fr>
 <c02ff60205fdb343cb5a2ff0e4384fc7b47635a3.camel@linux.ibm.com>
In-Reply-To: <c02ff60205fdb343cb5a2ff0e4384fc7b47635a3.camel@linux.ibm.com>
Accept-Language: fr-FR, en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?iso-8859-15?Q?09MMe/Xnw4v2WklIaTYkj2XhYbZZh1AYfxDx+EHKw0X3Ok7QsfM/VPTQT?=
 =?iso-8859-15?Q?WbwoS350j4HrDHycI42kEXTq2CEG0eoyS6L94SP3DfbXolPHDxwnDLPfg?=
 =?iso-8859-15?Q?hEO7If41J/pDmO2y8ynmqPggVSwagOY/V8VAyXLhZq5z2rRHkmj8GsOfW?=
 =?iso-8859-15?Q?+OgQirjnYf0+r4gwAEC83ejZoPPTSfhOQYqtdJ2w2EL/sbKzlfz9GnHzH?=
 =?iso-8859-15?Q?OSgJ+JfYabRY27C2FuaLiqbAHP1MllmrG7cTZEp3EiGb8RGe/IcrhYG0Y?=
 =?iso-8859-15?Q?ypVcOo5zImuko+DVed9plrKKS+nkErc3ivt7xHdzQl7R1X+DzliqAm8Wr?=
 =?iso-8859-15?Q?TlvbbreEXOvQXQR/HMxA2ctZwWlDSoDPOb1KIq9h/nRRIAL00A767jMJV?=
 =?iso-8859-15?Q?vA0XSYr2UpY0rjV8jTBF4/Ddn9D7257lSVP3x7fZTTbe2knifxLgX7+iu?=
 =?iso-8859-15?Q?kLF98wGcrKOySsB51qwWI4nS/2U3LLcy8BomaiBbhz1K/NtnI0MassqoQ?=
 =?iso-8859-15?Q?dBvmXNR9dDw2erY8qCiG22p5II5nqvYLm0IuJMt4QmGRBthD7Fr/ea5Bb?=
 =?iso-8859-15?Q?O5zHwbNQWENGCxdTSXLH/zG7V5MNj4OERhUXVXGNB7oEtUJR8xndZy2ps?=
 =?iso-8859-15?Q?RoCf0wX/MxP1oygaFyxGpIPXuBVl650zkgfCwWXxHXBVH9csz3qKftj1j?=
 =?iso-8859-15?Q?0+FqBjY3JIdWGPSdSnKUJNyHq5YU8hRtbYkaHfxzSeupqlP/MX+Tc9LJK?=
 =?iso-8859-15?Q?g2VjjNe/vVITAG+Z7hg/ejqE1IZfCFaAIMKal5z+Hfeucj8k7iSSV1Xw/?=
 =?iso-8859-15?Q?r76r5PJ3pwmOcoU+G8NMZaGHUH5cwRLUcAVHivC67zcvfzSrCB6WSQqyU?=
 =?iso-8859-15?Q?LHHdg15RAtRFIhnle3aA/a6QCJ5gVcViOuiJCrRhPTbSiI9ed4Pi1jGBU?=
 =?iso-8859-15?Q?Ikv2nRcdyDDRxjDtAKr7dA37rD+FKZc/LtK2GmPEKGBsRORzvcBNM8OgR?=
 =?iso-8859-15?Q?wm/1VyiQ8aDRvdqNBccmC0XrCUDyoZnt+zMFh9eaXZLfVGyYih6onKBA4?=
 =?iso-8859-15?Q?OAHUavTynUnB/mssE4qPhXNWdO3USDJMaHSDGfNoQ323+LG/qXqyluas2?=
 =?iso-8859-15?Q?B5855KTuqlBdqoSxZtFOAGrqJ6bnME1OhvZRD5VE9BvKIo5vSmqLzKfZc?=
 =?iso-8859-15?Q?vUk9VslMwq7VQFd/XMKqZe8FvnunSNAqrRRn3nI/tCMbkKB/hRu9x4Z2x?=
 =?iso-8859-15?Q?Qnu0f03SGjs4pUhzcunatQrAVwCO/9qWTvq8HKQh2Z0HttgyA/+uFS548?=
 =?iso-8859-15?Q?dzWQQotxYzwwuFT5zDRSH6C4pXZ+Pte4mk1zlne479U9yAHysuFSwCkzh?=
 =?iso-8859-15?Q?FzWsBEG++IK+kGW+Y4XyNn7Fnh+yn3Mfz?=
x-ms-exchange-transport-forked: True
Content-Type: text/plain; charset="iso-8859-15"
Content-ID: <B90E8D069B37F94DA0D302BFF30CBB19@eurprd09.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: viveris.fr
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: AM4PR0902MB1748.eurprd09.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 91d7a4b1-4e37-4b69-3e4c-08d9662dd83e
X-MS-Exchange-CrossTenant-originalarrivaltime: 23 Aug 2021 12:02:13.6055
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 34bab81c-945c-43f1-ad13-592b97e11b40
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: BrlH5p0yL/IV2YR9KzUsk800rdHI5oIbBNWYgNKgAxPOA+m6RYT/46jbOv+6Y+OYRSAuasGNcTnJRSPt8zDAeQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM9PR09MB4737
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Mimi,

On 8/23/21 1:57 PM, Mimi Zohar wrote:
> On Mon, 2021-08-23 at 08:14 +0000, THOBY Simon wrote:
>> Hi Liqiong,
>>
>> On 8/23/21 10:06 AM, liqiong wrote:
>>> Hi Simon :
>>>
>>> Using a temporary ima_rules variable is not working for "ima_policy_nex=
t".=20
>>>
>>>  void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos)
>>>  {
>>>  	struct ima_rule_entry *entry =3D v;
>>> -
>>> +	struct list_head *ima_rules_tmp =3D rcu_dereference(ima_rules);
>>>  	rcu_read_lock();
>>>  	entry =3D list_entry_rcu(entry->list.next, struct ima_rule_entry, lis=
t);
>>>  	rcu_read_unlock();
>>>  	(*pos)++;
>>> =20
>>> -	return (&entry->list =3D=3D ima_rules) ? NULL : entry;
>>> +	return (&entry->list =3D=3D ima_rules_tmp) ? NULL : entry;
>>>  }
>>>
>>> It seems no way to fix "ima_rules" change within this function, it will=
 alway
>>> return a entry if "ima_rules" being changed.
>>
>> - I think rcu_dereference() should be called inside the RCU read lock
>> - Maybe we could cheat with:
>> 	return (&entry->list =3D=3D &ima_policy_rules || &entry->list =3D=3D &i=
ma_default_rules) ? NULL : entry;
>>   as that's the only two rulesets IMA ever use?
>>   Admittedly, this is not as clean as previously, but it should work too=
.
>>
>> The way I see it, the semaphore solution would not work here either,
>> as ima_policy_next() is called repeatedly as a seq_file
>> (it is set up in ima_fs.c) and we can't control the locking there:
>> we cannot lock across the seq_read() call (that cure could end up be
>> worse than the disease, deadlock-wise), so I fear we cannot protect
>> against a list update while a user is iterating with a lock.
>>
>> So in both cases a cheat like "&entry->list =3D=3D &ima_policy_rules || =
&entry->list =3D=3D &ima_default_rules"
>> maybe need to be considered.
>>
>> What do you think?
>=20
> Is this an overall suggestion or limited to just ima_policy_next()?

I was thinking only of ima_policy_next(), I don't think (from what I could =
see in a short glance)
that other functions need such a treatment. The ima_rules_tmp dance is prob=
ably safe for the
other uses of ima_rules.

>=20
> thanks,
>=20
> Mimi
>=20
>=20

Thanks,
Simon=