From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.freebsd.org (mx2.freebsd.org [96.47.72.81]) by sourceware.org (Postfix) with ESMTPS id 957503858C62 for ; Thu, 30 Nov 2023 23:42:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 957503858C62 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=FreeBSD.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=FreeBSD.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 957503858C62 Authentication-Results: server2.sourceware.org; arc=pass smtp.remote-ip=96.47.72.81 ARC-Seal: i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1701387763; cv=pass; b=QUepLK0gKMQrVotDOHPceMrrVCXMjQvLgVfSnwRkHdYKJ5Ub53u9WiFD20oGlVdo8TMrbfWV/57E6dkF8mBlA7oLDB+etM+Av0/LYSzYrm9L98i69piZ7FZogOyRqzEvUvdoVASeolzp4uZ/I8aGDK6Eus5b40UI8LYdPI0Hcyw= ARC-Message-Signature: i=2; a=rsa-sha256; d=sourceware.org; s=key; t=1701387763; c=relaxed/simple; bh=vIypqvNz2roSRDAaEmU3G2TVKRQ/HBSYURgDgqDKD28=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=pnwshwS6M9p8wvOTVK5ouOtOn0ANRxMSlXyPOBpNTWy5W/qynfC0KdBVk8bGRNzB/gLnCHFxE8ba30yqQuL1a3Ga1spof5MaU+1i+A0C5vllwiFEp9M0AmsOrVegjaSPV82d3Q0/ZXKFz2KfjhlpF6ImTkDR7iMnSzXWoxv6i8s= ARC-Authentication-Results: i=2; server2.sourceware.org Received: from mx1.freebsd.org (mx1.freebsd.org [96.47.72.80]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) client-signature RSA-PSS (4096 bits)) (Client CN "mx1.freebsd.org", Issuer "R3" (verified OK)) by mx2.freebsd.org (Postfix) with ESMTPS id 4ShCRL5jMDz3Mh5; Thu, 30 Nov 2023 23:42:38 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4ShCRL4sHbz3grp; Thu, 30 Nov 2023 23:42:38 +0000 (UTC) (envelope-from jhb@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1701387758; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=yDXlrlt5JwbUq64GRHwS1Pa9kCoD0LIPgSQ51byZak0=; b=inLEKajwpd4bZwKoyp6O/W8Wr3E9lR6X2Mngpx/V/xDQRnJKutczeUuShWoStibvbYQJxZ Xnvc+cymsk1q+hfwm6XF0LQVsrYXQPMXYb94d36NKM7Wem8DrjtX8E3086JE3uBhh3fiOg sSFFIzXetvq7txnlTAunam19mvY8rmkoinIa6YbDzPRMjVnkWzmkyyCZRY7bbJd6+FalCW u08jCxH2faQYGeSy1/ike9fy1/u+iorU97npnGn224fcZzaSvFYWQCACRdZV+ph/7ouSX4 g1WDXWoTWui5TdV9UnIOeZ/dO50ZclmeuwBZATO0GAPYa9zr9MbwrYCmXcmEJg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1701387758; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=yDXlrlt5JwbUq64GRHwS1Pa9kCoD0LIPgSQ51byZak0=; b=S41Ei5U1lNmYRHBEwbG1o7i147Yw5cHfO57co7v+jHClXrwcqZfH067tjwPf9iea1qYyZQ sGtWFE8afHOa2UjxgeBLIbxmRV6yd/oCNygW4l5MrV8SCVP2TgOh1qrW2enzjG8YBy0t4b fkfWF+X8G1RF7Bvo1Q4J0BQ1vc0yh8jarxtPqYxvt+5zEPMEOG0+2eMz6UZPXegnYlliS9 Ta+x6VNZXkJWB23bAwIVsCJbTJVr8PXMSXJ6LyQdNtkXMBk/nIFPJvU5fX4fCA1HQG8bPd OGheEtSVZNS2rW7hgBxWP7Toa0hfL6zdZHMOp6zsz9ymXU+UlSnXFtwJFfTCfA== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1701387758; a=rsa-sha256; cv=none; b=K5adZm/xnk7q6HI8PHH84+Sav5A+NxdPE6CmcsIWMjNVFzStFw4rVRbaLRr7bEWkng8R/C NY+QVik+sVu9NSX3upXo7UaQ1UVS4pSPLSiHSaKSA7ZFrB9IPm8xRvAKl2Uyw12RLqfLNM DL2+civnbR4b0Y8Ax16/Hp8DNIuoflbYp6S7sq30AurfVHPHRV8+EmyQ66Oga1icbR7hvV Z6Db6XdcHBP8JSbaDkA9adDMGHaiT3UpCDoZTapLt5c7CCY4dn/MuSUgkiHWnhO4tDuov/ AjoH9AHhicqG8dvY2wq5wR2Qfs+GxVLPCk/MXlhwise6qL9dcwpEBJ+71UJnTg== Received: from [IPV6:2601:648:8384:fd00:b889:21c7:e7ab:a49f] (unknown [IPv6:2601:648:8384:fd00:b889:21c7:e7ab:a49f]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) (Authenticated sender: jhb) by smtp.freebsd.org (Postfix) with ESMTPSA id 4ShCRL0wG7z1K2; Thu, 30 Nov 2023 23:42:38 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Message-ID: <2fcb73af-37f0-40d7-aa4a-d6eac42f2ffd@FreeBSD.org> Date: Thu, 30 Nov 2023 15:42:36 -0800 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 1/2] gdb: return when exceeding buffer size in regcache::transfer_regset Content-Language: en-US To: Simon Marchi , gdb-patches@sourceware.org Cc: Luis Machado References: <20231130212057.722990-1-simon.marchi@efficios.com> <20231130212057.722990-2-simon.marchi@efficios.com> From: John Baldwin In-Reply-To: <20231130212057.722990-2-simon.marchi@efficios.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 11/30/23 1:20 PM, Simon Marchi wrote: > regcache::transfer_regset iterates over an array of regcache_map_entry, > transferring the registers (between regcache and buffer) described by > those entries. It stops either when it reaches the end of the > regcache_map_entry array (marked by a null entry) or (it seems like the > intent is) when it reaches the end of the buffer (in which case not all > described registers are transferred). > > I said "seems like the intent is", because there appears to be a small > bug. transfer_regset is made of two loops: > > foreach regcache_map_entry: > foreach register described by the regcache_map_entry: > if the register doesn't fit in the remainder of the buffer: > break > > transfer register > > When stopping because we have reached the end of the buffer, the break > only breaks out of the inner loop. > > This problem causes some failures when I run tests such as > gdb.arch/aarch64-sme-core-3.exp (on AArch64 Linux, in qemu). This is > partly due to aarch64_linux_iterate_over_regset_sections failing to add > a null terminator in its regcache_map_entry array, but I think there is > still a problem in transfer_regset. > > The sequence to the crash is: > > - The `regcache_map_entry za_regmap` object built in > aarch64_linux_iterate_over_regset_sections does not have a null > terminator. > - When the target does not have a ZA register, > aarch64_linux_collect_za_regset calls `regcache->collect_regset` with > a size of 0 (it's actually pointless, but still it should work). > - transfer_regset gets called with a buffer size of 0. > - transfer_regset detects that the register to transfer wouldn't fit in > 0 bytes, so it breaks out of the inner loop. > - The outer loop tries to go read the next regcache_map_entry, but > there isn't one, and we start reading garbage. > > Obviously, this would get fixed by making > aarch64_linux_iterate_over_regset_sections use a null terminator (which > is what the following patch does). But I think that when detecting that > there is not enough buffer left for the current register, > transfer_regset should return, not only break out of the inner loop. > > This is a kind of contrived scenario, but imagine we have these two > regcache_map_entry objects: > > - 2 registers of 8 bytes > - 2 registers of 4 bytes > > For some reason, the caller passes a buffer of 12 bytes. > transfer_regset will detect that the second 8 byte register does not > fit, and break out of the inner loop. However, it will then go try the > next regcache_map_entry. It will see that it can fit one 4 byte > register in the remaining buffer space, and transfer it from/to there. > This is very likely not an expected behavior, we wouldn't expect to > read/write this sequence of registers from/to the buffer. > > In this example, whether passing a 12 bytes buffer makes sense or > whether it is a size computation bug in the caller, we don't know, but I > think that exiting as soon as a register doesn't fit is the sane thing > to do. > > Change-Id: Ia349627d2e5d281822ade92a8e7a4dea4f839e07 > --- > gdb/regcache.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/gdb/regcache.c b/gdb/regcache.c > index 9dc354ec2b3a..e46a0b58f505 100644 > --- a/gdb/regcache.c > +++ b/gdb/regcache.c > @@ -1208,7 +1208,7 @@ regcache::transfer_regset (const struct regset *regset, int regbase, > for (; count--; regno++, offs += slot_size) > { > if (offs + slot_size > size) > - break; > + return; > > transfer_regset_register (out_regcache, regno, in_buf, out_buf, > slot_size, offs); This makes sense to me. In particular, note that a few lines below we do a return for the same condition when an individual register doesn't fit. Reviewed-By: John Baldwin -- John Baldwin