Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6c195897-e2c5-f0b8-45b4-83a0e9b71bf8@meta.com>
Date:   Fri, 9 Jun 2023 20:20:09 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.12.0
Subject: Re: [PATCH bpf-next v4 2/3] bpf, x86: clean garbage value in the
 stack of trampoline
Content-Language: en-US
To:     menglong8.dong@gmail.com, andrii.nakryiko@gmail.com,
        alan.maguire@oracle.com
Cc:     ast@kernel.org, daniel@iogearbox.net, andrii@kernel.org,
        martin.lau@linux.dev, song@kernel.org, yhs@fb.com,
        john.fastabend@gmail.com, kpsingh@kernel.org, sdf@google.com,
        haoluo@google.com, jolsa@kernel.org, bpf@vger.kernel.org,
        linux-kernel@vger.kernel.org, Menglong Dong <imagedong@tencent.com>
References: <20230609095653.1406173-1-imagedong@tencent.com>
 <20230609095653.1406173-3-imagedong@tencent.com>
From:   Yonghong Song <yhs@meta.com>
In-Reply-To: <20230609095653.1406173-3-imagedong@tencent.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ZUlxYnoyUURsbXF2OTA2QWVYQzZWejJseEMxSGNtSDNJbERMakdLbnd0R2cz?=
 =?utf-8?B?Um1CODFFL1VFNE1yelVzZW9HOXFrVi9uV1BoQ0pOa2lUZWgrQXo1ekphLzcr?=
 =?utf-8?B?cVpSOWRlS0FCbFFDRHRocnNTK2FHNmFCVWlxZHJzanBJSXhETEROQnpjUEQ2?=
 =?utf-8?B?RnJCcWIxTU5tSS96ZTFkVkFpR3lPWkVURUtKYkJaRWRBUHp0RzRNeEpwS1FV?=
 =?utf-8?B?SklFcDladHArVTMzZ1kzdy9McElleVBWeXhBU1RZZXRSQjJvd3hhSmpHS3Vy?=
 =?utf-8?B?S1ZWYzVhU2E4YjVBVWM1Vll2d1EvZW9KaEFRWVZDU3p5Z2VYcXBzMWs4VFBq?=
 =?utf-8?B?VzFXOFR3M21PL1hiZEpkN1R4NDNGRm1pRDhHcGFObEZpVGk0WWFrSy9iN1Fq?=
 =?utf-8?B?N0JGTTF2T0E3NW5acjh1Ryt0ZlhkbXJVY2VjWXJ1bzNsMm55QUp3N0FNSmJN?=
 =?utf-8?B?MHZZK2QvTllRWjVoTHNYeVFJcVBOMU5NVGp5T3N6TEhGbDBXZi9FV21yNHN4?=
 =?utf-8?B?anFWUmNESStZNW0yaHZtRXdOaHlIb1g4b3JCQ01kaExKSFQzNnBoNXZxaU84?=
 =?utf-8?B?V1p6MFlvQVRtbnliNDJFRUlmN2dheXZtY1BLeGVnUTQyWkM0T2UwOW1QelVt?=
 =?utf-8?B?UU5ELzUwMGNOdytzdTJNWHc1TW1yc0xNMDFZa1ZiYnRtUVhHRFR2MDJvUDky?=
 =?utf-8?B?dENrb3NMaldPcXRKMVg1NGtUSUlaWWxYdStnYjMxT2NZNG9uaUUzS1VqbjNP?=
 =?utf-8?B?WFpHMjR1M2J5SWJ2b3paaGpYU3R5Ni9JZGxkV3d1bktLUnhNUll1Z0JTRTBn?=
 =?utf-8?B?dXV5YS83Z0cxZDF0dWZZRHVESnRFS2hZUnpiWkJFM1hWT3ZnQXg3UHcyMFpL?=
 =?utf-8?B?QndoSEZMUzg2Z2Y0SGNvTzQxdzQwUUZFR1NFb1RXMkVmOWp0VENqemlJWDQr?=
 =?utf-8?B?eHhmbXlYSU15cWRFUjYwMVd5R2VKalBSRi93Ukc3MG1SWjNzTkx0alNmclNC?=
 =?utf-8?B?WXFobHVKL2xhUE9sZXhKV1Y5QXlBNXNTTGZuaks1THE4WlY5ZW9MM25hUmph?=
 =?utf-8?B?VUd0QVNqMXJVZlgwd0tLNWh4SUZDNU1FSG4zbGZXSk11Rm1JVzRveWZkTkFh?=
 =?utf-8?B?RkkyRUdDZ2dzZ1J3TURiZ1dMR0FQWGJLTVhXWlRQdE55akN5bmFMWCt4dFBu?=
 =?utf-8?B?YmVubHVOV0czYVBRRzlmSVg2VVlJSG9DZ3UxRWhxRlFEWTNUdGUzQklDUzNm?=
 =?utf-8?B?VjFlL0dvUlMvTk9BMzNzbWJ0Y090bXNYb2djeVU1T3dxZkdLSW01ODJqWnlB?=
 =?utf-8?B?dUF5dWptWkFVbXhndWVuajd0OFQ5TWhtZ2MwbFVFK2VvWi9nSno0Sms0TStl?=
 =?utf-8?B?dUFWTk0vbi81YzVEZTZJMFhoL1F5ZVUwZ2lvZHR0OUJjdDl0R1NNSTdxOXhP?=
 =?utf-8?B?Qk1DaWl4TE4xQkFHWEZETzg1MkNDcmFTNUNoUk9JMGFDL1NWVkpXYkYvSGUy?=
 =?utf-8?B?b0FVSi9oSThBYmJqR1lFeFFiWWxBMHp2L2k1TWcxS05Qa2RwWDdXZyt2bmNm?=
 =?utf-8?B?Sjl0aDFnZGdtNHV5VWNyMTlXVTFuT3A0NElUcGJvcDRHWmEyWit1bUExdEhS?=
 =?utf-8?B?ZGhFcHZkb01NMkw3Mm5SWkROR00xbjEweXUwT2hTWWdJUWZrOC9hbTNlaTNx?=
 =?utf-8?B?Y2lURENiSkdjM2FETVBIR2FwazFManpUa3NCMzNFalpnOGRLbUlDcDhVQ21i?=
 =?utf-8?B?RERLc1l0K0dXOUVaclpDK1dvaWZ1aU5lRjNTd2JoQmZBbk1FblBwSDhGZ0FO?=
 =?utf-8?B?ZWxZSmFFbEcwTGsrdFc3ZVlrb2VySXhTaEg2ZDlDTG4wcmcrVFBUZ1RYY0xH?=
 =?utf-8?B?cmlSRTM5UDNPaUpUdk9Ccy9wTEIwV3JRSGx4dFllNGRnb0lJeTNWMHRPdXow?=
 =?utf-8?B?WVRCenh1ZTFheENFbmJ3NGZKK2lWUXFyTFRPYkRKd293Y2xsOHU4VW8vc0hs?=
 =?utf-8?B?b0Fqa0FIV0g0V2x2bTNkOC9oZmpkTTg2UWU5UTFtNTNlOVAyU05KekV0b2Vt?=
 =?utf-8?B?V1F5eGhaVy85eU85c1dTL0FYQnRzOTIxMm9lK1pZbVBHK01ycUJKSmx3RWo5?=
 =?utf-8?B?dXlaN0hEa0RGNVZFTXlEWEkvMzMwL0lYeUV1WnpqMUs2WnRBaFlFWk93a3dW?=
 =?utf-8?B?Ymc9PQ==?=
X-OriginatorOrg: meta.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e853acfc-e893-4983-5de8-08db69619ad5
X-MS-Exchange-CrossTenant-AuthSource: SN6PR1501MB2064.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Jun 2023 03:20:13.4363
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: CsZwm5s/bnmTOhmV/sLjnRtgiKkqj8bK6CfvNMDY2jhBPKaIYb2pzijfP/9MN4f0
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR15MB4149
X-Proofpoint-ORIG-GUID: Rd89957PtYcSU-wSmLO698quPRovYBd6
X-Proofpoint-GUID: Rd89957PtYcSU-wSmLO698quPRovYBd6
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.573,FMLib:17.11.176.26
 definitions=2023-06-09_18,2023-06-09_01,2023-05-22_02
X-Spam-Status: No, score=-2.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_PASS,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 6/9/23 2:56 AM, menglong8.dong@gmail.com wrote:
> From: Menglong Dong <imagedong@tencent.com>
> 
> There are garbage values in upper bytes when we store the arguments
> into stack in save_regs() if the size of the argument less then 8.
> 
> As we already reserve 8 byte for the arguments in regs and stack,
> it is ok to store/restore the regs in BPF_DW size. Then, the garbage
> values in upper bytes will be cleaned.
> 
> Signed-off-by: Menglong Dong <imagedong@tencent.com>
> ---
> v4:
> - clean grabage value when argument count is 7
> ---
>   arch/x86/net/bpf_jit_comp.c | 45 ++++++++++++++++++++++++++-----------
>   1 file changed, 32 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index a767e13c8c85..f6f51a5d14db 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -1857,6 +1857,28 @@ st:			if (is_imm8(insn->off))
>   	return proglen;
>   }
>   
> +static inline void clean_garbage(u8 **pprog, int nr_regs, int stack_size,
> +				 int arg_size)
> +{
> +	u8 *prog;
> +
> +	/* clean potential garbage values in upper 32-bit. 'stack_size'
> +	 * here is the offset of the 7th argument on-stack.

Here, we have a huge assumption that if only 7 registers, compiler might
just allocate a 8-byte stack space and write the value to it. If the
type of the value is <= 32bit, a 32bit store will be on the stack.
So in this case, the upper 32bit needs to be cleared.
If the number of arguments (in terms of number of registers) is more 
than 7, the extra arguments will be 'pushed' to the stack, so there
is no garbage. This could be true. But please add enough details
here so people knows why we special case this particular instance.

> +	 */
> +	if (nr_regs == 7 && arg_size <= 4) {
> +		int off = -(stack_size - 4);
> +
> +		prog = *pprog;
> +		/* mov DWORD PTR [rbp + off], 0 */
> +		if (!is_imm8(off))
> +			EMIT2_off32(0xC7, 0x85, off);
> +		else
> +			EMIT3(0xC7, 0x45, off);
> +		EMIT(0, 4);
> +		*pprog = prog;
> +	}
> +}
> +
>   static void save_regs(const struct btf_func_model *m, u8 **prog, int nr_regs,
>   		      int stack_size)
>   {
> @@ -1878,8 +1900,7 @@ static void save_regs(const struct btf_func_model *m, u8 **prog, int nr_regs,
>   
>   		if (i <= 5) {
>   			/* copy function arguments from regs into stack */
> -			emit_stx(prog, bytes_to_bpf_size(arg_size),
> -				 BPF_REG_FP,
> +			emit_stx(prog, BPF_DW, BPF_REG_FP,
>   				 i == 5 ? X86_REG_R9 : BPF_REG_1 + i,
>   				 -(stack_size - i * 8));
>   		} else {
> @@ -1893,17 +1914,16 @@ static void save_regs(const struct btf_func_model *m, u8 **prog, int nr_regs,
>   			 *   8(return addr of the caller)
>   			 * which means: rbp + 24
>   			 */
> -			emit_ldx(prog, bytes_to_bpf_size(arg_size),
> -				 BPF_REG_0, BPF_REG_FP,
> +			emit_ldx(prog, BPF_DW, BPF_REG_0, BPF_REG_FP,
>   				 (i - 6) * 8 + 0x18);
> -			emit_stx(prog, bytes_to_bpf_size(arg_size),
> -				 BPF_REG_FP,
> -				 BPF_REG_0,
> +			emit_stx(prog, BPF_DW, BPF_REG_FP, BPF_REG_0,
>   				 -(stack_size - i * 8));
>   		}
>   
>   		j = next_same_struct ? j : j + 1;
>   	}
> +
> +	clean_garbage(prog, nr_regs, stack_size - 6 * 8, arg_size);
>   }
>   
>   static void restore_regs(const struct btf_func_model *m, u8 **prog, int nr_regs,
> @@ -1925,7 +1945,7 @@ static void restore_regs(const struct btf_func_model *m, u8 **prog, int nr_regs,
>   			next_same_struct = !next_same_struct;
>   		}
>   
> -		emit_ldx(prog, bytes_to_bpf_size(arg_size),
> +		emit_ldx(prog, BPF_DW,
>   			 i == 5 ? X86_REG_R9 : BPF_REG_1 + i,
>   			 BPF_REG_FP,
>   			 -(stack_size - i * 8));
> @@ -1956,17 +1976,16 @@ static void prepare_origin_stack(const struct btf_func_model *m, u8 **prog,
>   		}
>   
>   		if (i > 5) {
> -			emit_ldx(prog, bytes_to_bpf_size(arg_size),
> -				 BPF_REG_0, BPF_REG_FP,
> +			emit_ldx(prog, BPF_DW, BPF_REG_0, BPF_REG_FP,
>   				 (i - 6) * 8 + 0x18);
> -			emit_stx(prog, bytes_to_bpf_size(arg_size),
> -				 BPF_REG_FP,
> -				 BPF_REG_0,
> +			emit_stx(prog, BPF_DW, BPF_REG_FP, BPF_REG_0,
>   				 -(stack_size - (i - 6) * 8));
>   		}
>   
>   		j = next_same_struct ? j : j + 1;
>   	}
> +
> +	clean_garbage(prog, nr_regs, stack_size, arg_size);
>   }
>   
>   static int invoke_bpf_prog(const struct btf_func_model *m, u8 **pprog,