Message-ID: <2941c2b9-5fa5-e25c-dcd0-ab9c9c0f143e@oracle.com>
Date:   Tue, 21 Feb 2023 08:30:11 -0500
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH] vc_screen: don't clobber return value in vcs_read
Content-Language: en-US
To:     =?UTF-8?Q?Thomas_Wei=c3=9fschuh?= <linux@weissschuh.net>,
        Jiri Slaby <jirislaby@kernel.org>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org, Randy Dunlap <rdunlap@infradead.org>,
        Storm Dragon <stormdragon2976@gmail.com>,
        Linus Torvalds <torvalds@linux-foundation.org>,
        sfr@canb.auug.org.au, akpm@linux-foundation.org,
        linux-serial@vger.kernel.org
References: <Y/KtG9vK0oz0nQrN@hotmail.com>
 <20230220064612.1783-1-linux@weissschuh.net>
 <dcaaf7d5-b2d5-dbb9-f3fe-2232ee525cc8@kernel.org>
 <2094ecec-f63c-4e8a-ba97-da77c5266da1@t-8ch.de>
From:   George Kennedy <george.kennedy@oracle.com>
In-Reply-To: <2094ecec-f63c-4e8a-ba97-da77c5266da1@t-8ch.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SnIrM3NPNlVWaG5PVWFmS3RNcFExR2g5VXdXYmJKVHhiSWx1R1VlaHBESWM3?=
 =?utf-8?B?UGg1cGdheHIycFg0SGJYSElvK0pYUmRyVVZHZ2h2YUhPVEVsdlNWTlpmVzQx?=
 =?utf-8?B?bDRJZVk0cHovSlBhNTlRMmdrY3VubjJiYVlCNnVKY2lQdWxoM0xxU3ErTGN1?=
 =?utf-8?B?OUlhWGI3QWUxRjR2M1dqemZ0ZVlUSnBRdkg3dnF4TmR6MitPME43MllFWVRJ?=
 =?utf-8?B?OFNNSHREaXN3TVBpUEs3M1FxWDJVSG1WQkNtS2FEeUY5Y0E3MEJWNmpGSGVh?=
 =?utf-8?B?VWZvbnp0R09Bbi9Vc1ZTVVV3L041eGw5KzR6cXdlczdSemY2RkRlWFpBNnJU?=
 =?utf-8?B?SlVSdTArTWpHYnU4UmxaeWJMaWU4Q2hYbXJpN0IrdTVnQmIwK1BPMmRKeTdU?=
 =?utf-8?B?UDE2bVFESFI2UTg5d0hGYTBqOWhLRnV1NnRNM0xNZ3pmbkRhTmlydGZNeUI0?=
 =?utf-8?B?U0JzL2VVWGU0b2pqYXpwZUIrRXhlT1NqVFh3Ly83SWhUb01VZzJKT0tXRStV?=
 =?utf-8?B?MWNOaDVqZ2ZUYytDUEhzWWJudXpvRVdkbXNFMWlLN212YnBsd29qZmdMUEtK?=
 =?utf-8?B?NnhZY3M5WUNQUDd2RFljVkxWTG42OFRqSzZQM0wzbkdEczhEYi9jampYSzh2?=
 =?utf-8?B?L3kxblB6aDhkOUdpQ1RGbEJlelhBaVYvSzNjWDE0WVdtbk1xV0dSWXdwSXJX?=
 =?utf-8?B?VTZRQXhyeUtOQlhnaDZoOHpFVC9laXdQU3Q4NWFmei9WbmhldGQxSUhZRC8z?=
 =?utf-8?B?L2I1UUJUOTZHTlU5a0NuUHhqQ0o0cE9qdkFXZ09BK1R4Y2ljOHQ2TGl0RFVP?=
 =?utf-8?B?NUJhWmVEbUt6OXZEWVNxcTdsT1REc0ZZQUJvY3ZzQVFKNjJZTHRRcmlTa1lD?=
 =?utf-8?B?TFk4elFMWFZocitGRHVNdHNEdUhGcnBRTkpMa08yNTFHWDJMNDFzbWRNaUcx?=
 =?utf-8?B?blk5dUtVQ0Y2aCtTNU5QenNhR3JSWUhlcnhlQ2hPYWV1Vk5GYi9MbEdjc01X?=
 =?utf-8?B?NEw1aVBydDBtYm4yTjFnTjJQa3IxSHhMbnBjQU1WUlIwdkUrbUtQUUhzLytZ?=
 =?utf-8?B?YVFtKzVkbEgrTXVZdmZ4cmpmSDhwQjcxVE0xb0lDdVQ1R243dytXZ0ttaVor?=
 =?utf-8?B?RzVlNDJVSWk3M0RaV2hNMmJHWEg5L0wvZEdUY0s3a08xakJMalFLRDQwS1F0?=
 =?utf-8?B?K3AwYXJRZGlVT3VLekJ0Wi9yZmtTOXdUWkswUXlnS0dkMFRLRkMzaERYeG9B?=
 =?utf-8?B?TVhldGs1M3JHVkJCUVNnK3V1dnZ4aitDckoydmFrVUlGZnhQQ3ZDeEVYSmpj?=
 =?utf-8?B?cUZMOVh1a3FYWHVSN0RuMWpnZzFMSkxXU3dsTVZ6MU0zYm12N3lTMUdFQ1FC?=
 =?utf-8?B?ci9EZmVZcEU0blBuTnNWaVhKczZmd0RsTXlGZTZwNmR3TTV6aFlnRndVSWZu?=
 =?utf-8?B?UkdRWXBJSUJyanAwU3R6ZGx6amNaOFAydUszRythai9IdWxGSENEbCtxWWM4?=
 =?utf-8?B?aEQwL0ljZXdaOXpHT2Fhc3B4OFJSSVpldldkL0hGSjZOeWh4Q1ljOUZJRnpl?=
 =?utf-8?B?WjZCQWhuK2xTVWpBQ3BoRlZZamZmZlZ0TUZjK0RGck5PRTlsKzBpcmRRSXFM?=
 =?utf-8?B?S0ZRSG1QWVZjOGF3dlhJc0I5a2pYelVocFRUV21jMERPLzRQUU5GRnpPcHRs?=
 =?utf-8?B?Ym5sTStSdFl3bnU0V2xCV0VDZk0wVTRCVnpldERNZkVTMUtJZlZialhUNkdW?=
 =?utf-8?B?ZnV5cnNpRlJmL0UyMVBKQ0FXZ2dPVXd5Zjl5VHl1VDJpUkQrcEVsRzA1S0Fn?=
 =?utf-8?B?UmkwZG0zREdSR1lEdGozZW1wbjJhblVPR2s4cHE4Ly9TN3VGOUZCU213SjFn?=
 =?utf-8?B?SnVzeDNkdG90eTZZT3lyREQ1S2FmK2wydStXeDI0ekU3eUt6NHJMOWxaNzlC?=
 =?utf-8?B?Q2VOd2YvYytvcGE5VnhlSmNTa2FmTDVYMUEvREZYb1ZISUpzbUV2SFJJYTFj?=
 =?utf-8?B?S3RRTzgwZEJybGloTCtGcHdZZkZ1Z3kzMnJSYWVHclI3T2JyZGNDUE1BOUh2?=
 =?utf-8?B?MTdjeVdNU2pUQ2Y2ZUo4UG8vdFI5U3lzMXN3NWZhRGRJYmQ2VUlDcGU5blNZ?=
 =?utf-8?B?clZkWEVqWS9xaDN6UUVmZmhUalJBYkZGSmtLN0tvcUF2aU41bmVmc0dtVmVE?=
 =?utf-8?B?WVE9PQ==?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?utf-8?B?QzlzNU1ZUk9EaFpFbTBTaDlQcnc0UXpOQ1U4d0JQNThMSVlQM0xnbnhVVzJK?=
 =?utf-8?B?di83YlFEdjNUODN4eGRUT3BZak5nd2FTR2hzcHk1VjJjN3B0UHM2d2I4Vk9Z?=
 =?utf-8?B?VjdtakNoTlRvRE85QmQ1WEE3MXpvSjF2VDdnRE5nVlBtMWFhaFhXRmU0QVk2?=
 =?utf-8?B?TXNCZmRKN1ZiVHhRNFRXY0hwREkrS0E4a01Zc0UzU29RdnhTRXlHVXZMRzVt?=
 =?utf-8?B?UEhkaTJ5U0tUcnRRRDZ5NldZS3VGM01yK1laN0xhTWlZUDJQcDhjVVVmWk9G?=
 =?utf-8?B?ZXhRZmRZOHJWVThtdGxxTHJxcWx6eVg5YTJJQWxYaXpRUGNVMHloeHR1Qmxy?=
 =?utf-8?B?L2pEcE5FMnhWU2h0MXM4d20zblhMQ3RLbFMzWlpyempPMzZMd2pkdW1MK09V?=
 =?utf-8?B?aE96Nzh2RWZYVFlpZlBSYUFOZjVPSmR5M0cvVS9CNHQzblRJcHhqeGdseTY2?=
 =?utf-8?B?ZzFpMkxtd1Vlend1RGVZR2wwdGlOZFdnT29kc0tkS0l6NENpb1BJWmI1cGYy?=
 =?utf-8?B?Y0hqTkNkVi95TU5ySVZ0cFF0KzlBZWp1VDdGUDlVWGZqUmg4TEpBNWVBMEZl?=
 =?utf-8?B?RURGWXBQdityWmFzdHUrZC9ocTJqSGtvUTNBT1JqM1MraFhBakhhNTl5STQz?=
 =?utf-8?B?TnN4SEtpSXB5L1BxY0J5aFNHeGZpbEVWQ0dGdlp4MXpQOXBrYlVXdGNrTGZm?=
 =?utf-8?B?SmlYQW0vZVBnbk1qTFB6MkhmcDNXUWNIdTcrWEJrTW4zQXJrWllhQXFYeUtN?=
 =?utf-8?B?WkNhaGtUUTRHTndTRW5xclJSaTkvUFhPd29md2dpTURZcmIzeGpLVmVSNnMy?=
 =?utf-8?B?R3hkellkdFNaMXRJYktsSStvZEVKUjloR3VVek4rOSs2bXFLWEtrcmVodVFi?=
 =?utf-8?B?ZmlSSHJNRXI2eEJyeFpKY1dHOEJrRmdHRnQ1TkxJQ3h4eUszSFpxRDN0Qlly?=
 =?utf-8?B?UnhYR1BicXJuWXFyU3ExVFVCbll6ZzN6OG9KWkEvcFdpa0VZYktoc083Nm1l?=
 =?utf-8?B?MndLS1NZbWlmeEh2bC9JTitTUE81bmtxRlErdFg0N0lXeHBCMkVidG93UEM3?=
 =?utf-8?B?QUY5V09BQUsrL2Z5T3Q4YjhSeHRXWVpSTnpoWDlRd0RJN2FZeVhEM2crM1Mr?=
 =?utf-8?B?M2dHQklhbzhRN0Y3dmNQU2ozaEJwNE5GdVNSVkFhcUw2TTFWb1pnV1dqL3ZO?=
 =?utf-8?B?VlhFTFU2cTJQOC9wMkZJenVzdzFtZGl4STlwWUlkc3BacG1UZm1PUHBFS2pE?=
 =?utf-8?B?TTd6a1d4aElqOGhOc0o3L3VCME11Ni9LeGI3ZmQ1V2cwNEVBc1ZLeE9TR0J0?=
 =?utf-8?B?MENMbGUvVzBJS0NocWcxVC9OVEp0VWlXbWNvWGJRV01MeG1HWEg0Q3JUank4?=
 =?utf-8?Q?hQABhPUa2TngqkcOc+hn/WD0lATSKnzs=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 91e5ca1e-9bf4-43ea-112c-08db140fded8
X-MS-Exchange-CrossTenant-AuthSource: PH0PR10MB5895.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Feb 2023 13:31:00.0542
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: JtQLUJDGZ1zQ2F4wORbNX5RM0qc4lS3YAbC/RShsQJ3Enej0SIK+6gZGPz0fH731JR5bGhAldeA5O3lMKgcGCgAZ2TXMCMz8h3+cOS15IPg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR10MB5131
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.219,Aquarius:18.0.930,Hydra:6.0.562,FMLib:17.11.170.22
 definitions=2023-02-21_08,2023-02-20_02,2023-02-09_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 adultscore=0 phishscore=0
 mlxlogscore=999 bulkscore=0 suspectscore=0 mlxscore=0 spamscore=0
 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2212070000 definitions=main-2302210115
X-Proofpoint-GUID: ljSmHjBvHjMrKnqx045zvwR8iiuHofoD
X-Proofpoint-ORIG-GUID: ljSmHjBvHjMrKnqx045zvwR8iiuHofoD
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 2/20/2023 11:34 AM, Thomas Weißschuh wrote:
> +Cc people who were involved in the original thread.
>
> On Mon, Feb 20, 2023 at 12:48:59PM +0100, Jiri Slaby wrote:
>> On 20. 02. 23, 7:46, linux@weissschuh.net wrote:
>>> From: Thomas Weißschuh <linux@weissschuh.net>
>>>
>>> Commit 226fae124b2d
>>> ("vc_screen: move load of struct vc_data pointer in vcs_read() to avoid UAF")
>>> moved the call to vcs_vc() into the loop.
>>> While doing this it also moved the unconditional assignment of
>>> "ret = -ENXIO".
>>> This unconditional assignment was valid outside the loop but within it
>>> it clobbers the actual value of ret.
>>>
>>> To avoid this only assign "ret = -ENXIO" when actually needed.
>> Not sure -- I cannot find it -- but hasn't George fixed this yet?
> Indeed there was a proposed fix at
> https://lore.kernel.org/lkml/1675704844-17228-1-git-send-email-george.kennedy@oracle.com/
>
> Linus had some suggestions so it was not applied as is.
>
> I'm not sure what the current state is.
> George, do you have something in the pipeline?

Yes, that is in the pipeline:
https://lore.kernel.org/lkml/1675774098-17722-1-git-send-email-george.kennedy@oracle.com/

Linus suggested the fix, which was tested and submitted.

Jiri commented on the patch, which I believe was directed at Linus as he 
suggested the fix.

George
>
> I also tested the patch proposed by Linus as attachment and that works.
> (The small inline patch snippet doesn't)
>
>>> Reported-by: Storm Dragon <stormdragon2976@gmail.com>
>>> Link: https://lore.kernel.org/lkml/Y%2FKS6vdql2pIsCiI@hotmail.com/
>>> Fixes: 226fae124b2d ("vc_screen: move load of struct vc_data pointer in vcs_read() to avoid UAF")
>>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>>>
>>> ---
>>>
>>> @Storm Could you validate this patch?
>>> ---
>>>    drivers/tty/vt/vc_screen.c | 5 +++--
>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/tty/vt/vc_screen.c b/drivers/tty/vt/vc_screen.c
>>> index f566eb1839dc..2ef519a40a87 100644
>>> --- a/drivers/tty/vt/vc_screen.c
>>> +++ b/drivers/tty/vt/vc_screen.c
>>> @@ -403,10 +403,11 @@ vcs_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>>>    		unsigned int this_round, skip = 0;
>>>    		int size;
>>> -		ret = -ENXIO;
>>>    		vc = vcs_vc(inode, &viewed);
>>> -		if (!vc)
>>> +		if (!vc) {
>>> +			ret = -ENXIO;
>>>    			goto unlock_out;
>>> +		}
>>>    		/* Check whether we are above size each round,
>>>    		 * as copy_to_user at the end of this loop
>>>
>>> base-commit: c9c3395d5e3dcc6daee66c6908354d47bf98cb0c
>> -- 
>> js
>> suse labs
>>