linux-kernel - Re: [PATCH] virt: acrn: fix invalid check past list iterator

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <6E68C33F-9CBB-418C-A11D-2AD863C0B19A@gmail.com>
Date:   Thu, 31 Mar 2022 13:20:50 +0200
From:   Jakob Koschel <jakobkoschel@...il.com>
To:     Li Fei1 <fei1.li@...el.com>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Mike Rapoport <rppt@...nel.org>,
        Brian Johannesmeyer <bjohannesmeyer@...il.com>,
        Cristiano Giuffrida <c.giuffrida@...nl>, h.j.bos@...nl
Subject: Re: [PATCH] virt: acrn: fix invalid check past list iterator


> On 30. Mar 2022, at 09:57, Li Fei1 <fei1.li@...el.com> wrote:
> 
> On Sat, Mar 19, 2022 at 09:38:19PM +0100, Jakob Koschel wrote:
>> The condition retry == 0 is theoretically possible even if 'client'
>> does not point to a valid element because no break was hit.
>> 
>> To only execute the dev_warn if actually a break within the loop was
>> hit, a separate variable is used that is only set if it is ensured to
>> point to a valid client struct.
>> 
> Hi Koschel
> 
> Thanks for you to help us to try to improve the code. Maybe you don't get the point.
> The dev_warn should only been called when has_pending = true && retry == 0

Maybe I don't understand but looking isolated at this function I could see a way to call
the dev_warn() with has_pending = false && retry == 0.

> 		list_for_each_entry(client, &vm->ioreq_clients, list) {
> 			has_pending = has_pending_request(client);
> 			if (has_pending)
> 		}
> 		spin_unlock_bh(&vm->ioreq_clients_lock);

imagine has_pending == false && retry == 1 here, then client will not hold a valid list entry.

> 
> 		if (has_pending)
> 			schedule_timeout_interruptible(HZ / 100);
> 	} while (has_pending && --retry > 0);

since has_pending && --retry > 0 is no longer true the loop stops.

> 	if (retry == 0)
> 		dev_warn(acrn_dev.this_device,
> 			 "%s cannot flush pending request!\n", client->name);
client->name is accessed since retry == 0 now, but client is not a valid struct ending up
in a type confusion.

> 
> If retry > 0 and has_pending is true,  we would call schedule_timeout_interruptible
> to schedule out to wait all the pending I/O requests would been completed.
> 
> Thanks.

Again, I'm not sure if this is realistically possible. I'm trying to remove
any use of the list iterator after the loop to make such potentially issues detectable
at compile time instead of relying on certain (difficult to maintain) conditions to be met
to avoid the type confusion.

Thanks,
Jakob