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: <0cf50c32-ab67-ef23-7b84-ef1d4e007c33@fb.com>
Date:   Fri, 20 May 2022 17:25:36 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.9.0
Subject: Re: [PATCH bpf-next 2/4] bpf: verifier: explain opcode check in
 check_ld_imm()
Content-Language: en-US
From:   Yonghong Song <yhs@fb.com>
To:     Shung-Hsi Yu <shung-hsi.yu@suse.com>, netdev@vger.kernel.org,
        bpf@vger.kernel.org, linux-kernel@vger.kernel.org
Cc:     Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <kafai@fb.com>,
        Song Liu <songliubraving@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>
References: <20220520113728.12708-1-shung-hsi.yu@suse.com>
 <20220520113728.12708-3-shung-hsi.yu@suse.com>
 <f9511485-cda4-4e5e-fe1f-60ffe57e27d1@fb.com>
In-Reply-To: <f9511485-cda4-4e5e-fe1f-60ffe57e27d1@fb.com>
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?eUNpeDhxNVhFNmkxNS9oMU5MTGV6eW9ORTZ2QlNIcmhYWm83aFF6TFdKb21V?=
 =?utf-8?B?dTFmcDdxOS9DWkFRWWVkS2Zmd1dkQUE3TUFDbDNEVFVob3M2dEo3a3J4K1Fw?=
 =?utf-8?B?TTgzN0U2UnZFbWcxWDk0NFovYnphcm9ueStQWWRxcjhSQy9pckFsZm1hS2ZG?=
 =?utf-8?B?Qzg1N2JXb0FYSm5EWmJZempsVkRCU0I5azJFY0FLNFBPRlFXRTFNVHlmWDB0?=
 =?utf-8?B?TnAwK3FMSGhwa2ZkS1FCNnNVUHAva2pGTy81Qk1NZEUxZ3hFaTVYaDRsZGNR?=
 =?utf-8?B?V25Cc1hySC9GVDZGcUd2azA1RDJzTzI5YXJxR1B3b0NkZWZJcUtQVmxScWtX?=
 =?utf-8?B?YkhDU3lkYXhhTHhuS0NsQm1mM3lxSFI5cnNmUzNsUkJPcTB1cnBNTW9wVENN?=
 =?utf-8?B?TWRQOXp6VEppRTE5ZE0zeTA3bUpTMUNVajNMemdvQldaNENscHlySkcyOUEr?=
 =?utf-8?B?V1orZUF0NjBJbXcxVndoZE1WZEpFSTJvbkN6ZEd0bUFuOG1jZGQvWE1IVDE3?=
 =?utf-8?B?bWR3cEJDaTJ6RTJ1enZacEo1MTB4bzJXTXNXdkVqTDEwem16VWRyOW1lL1Fu?=
 =?utf-8?B?RlcwOXlQK3ZxNlBTMGhkMVZudHcySTU3NnZ1S3JPUW1BbWtGcUVFcFBLOG95?=
 =?utf-8?B?dEh0VFFmQ2Q2a0ZHcUIxRlBFT0NHZDFFZkJNMmxrVkgvZnM5dDVVcHVUMGlF?=
 =?utf-8?B?SXdXVGk4dUYvOU9sc21XR3VVZjVGOHkzQ0s1OUYvRVo1NGhXR0Vycjc0aFRn?=
 =?utf-8?B?T2d0dE9nbXVDS1pqek5FSDNXWm9EMjg0RFVZdWxBTnFFOVJOWnB2VU5vWUNp?=
 =?utf-8?B?aXI2WHhRVEtoWFd5RzZ0Yk40bTF6Z3RTSE15MEg1dk1zT3hER0swclFXcktm?=
 =?utf-8?B?MDFVWmpSS3d3TzQ4VERtR29pSHpwS2ZZVnFTS3hyQW1zN3FndHhod2x0RGxo?=
 =?utf-8?B?NDllNVVocmpUcGJZTURNU1MwMWhidDZidVg5b2tia3hLZUJCTjFSTDJEUzZs?=
 =?utf-8?B?UVJta0haQnR2TWNUOXY2WkxLeXR3NmZoaDhmSHkralBoaHhFWVYvUnJMbTlN?=
 =?utf-8?B?WG5VZ1dkSmFZa09XaTl6UWlCc3h0UXBvTjJMdk9tdStsb0IxaS9jVm9oTUdv?=
 =?utf-8?B?Z25uMDNFQ25sYjM5T1dHcE5DRWhlSER4c3JQMlFPMDRyWjBPUytuQVIwVG56?=
 =?utf-8?B?RFA3TEdkZFZNYVhJa21nNCtQdyt5MHNSZElUeUhPUUxRK3psTCt3QmNUSG5K?=
 =?utf-8?B?ZXFaVWNRbE1pdlg1bFliSUsrK2sydDBJaWdvbTFhWXYyWWRkeXpIVGliK25o?=
 =?utf-8?B?VUxjUmdwaE9OUFdZOGk2U2pMKzErMkRubXV0RTNzT2RrOWdicEFPY3hlbHJP?=
 =?utf-8?B?bUNSRFdEUHNRNWdvVGoraldBY3hnZk5BRmRYdEtQNVVCSGNnbUp4VnNpQTZB?=
 =?utf-8?B?TGVpc2NhclhMd0FXMUtGMHRRTm16cys5V24wUjN5bWxXaEJyVW5JSkJBd0Zt?=
 =?utf-8?B?Qk5PYUllMmlmWFIrWUdNNlp6SzVSWUhTT1VJZTJ2d051RSt0WnVzY3JwYWcw?=
 =?utf-8?B?NDRnOWRabkJHa0dpV2ZCWXZtVWJPNmpjM0Jkb01aaXZVeUV1N2x0SGtVOUJ5?=
 =?utf-8?B?aml4dW1hVXJzaVVsbHBiajlhY0VnZ1Z4VWpHa3hiTEtONHdWTUswbmJUd1F6?=
 =?utf-8?B?Mjc2YkJRRmQ3SXVCSm1iT29FOVVwZGNEeW9ZbnNsTWhFTk5vcnZ0OVFjNW5l?=
 =?utf-8?B?aGw5R0ROMk9kV1ptNjFmVWdsMTNBdFhGaW9vSitxY2JQNGFSdlU2YUx1cHRO?=
 =?utf-8?B?bVRTRkZLbUVmMEJ2aHJEeERlS3kybk1JMkY2MVJUR00xUEZaaGlKM2NveUhv?=
 =?utf-8?B?VXhIL0hHaXQ4ZnlQQUF5bEg3b1VIeUlySDhGUU96ZU12U1ZJbWNYU2NKMU12?=
 =?utf-8?B?d0o0cFAzWlA2eVhxMmpyNlliUU5MNWtvd1dPclkvU0RTTXlsM2VCeTFrdVFy?=
 =?utf-8?B?R3lRVUFENlI2SlpBYll4Rm4wU1JLQzljWVZ3OFUzb3lZbTlBaDFlK282SFd2?=
 =?utf-8?B?Y2pCR01YSGxqRm9rR1Y2c1l1QnFaQ05ESXFXVy96dkhpS1JJSWpKU3hXanl1?=
 =?utf-8?B?ekRoVXV6RFlrYzFjRVAxQzFYcnN2VHJkZHprQkN4dWx3RDBUKzBPcmFiOE1U?=
 =?utf-8?B?TTdRSmtQRGJPMnFYNTh5YlJRUGpzM3JNNmJBUlBpaEMyNWNwZ2ZndVF2U2h6?=
 =?utf-8?B?N1F6VGEzcStRUmVYd1lFSVdCS3ZzMllEem5IVjRoY3BuZlcveWxYd05Oajdz?=
 =?utf-8?B?ZzBYdDIyRkpJMDV2SWZFNTJ2NkdjTWNYSktpcHJDUTBpempYd2NvYXE0ZDFL?=
 =?utf-8?Q?JceA275BEgLUm3+g=3D?=
X-OriginatorOrg: fb.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 157a102f-cebf-459e-fa0c-08da3ac06e38
X-MS-Exchange-CrossTenant-AuthSource: SN6PR1501MB2064.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 May 2022 00:25:38.5268
 (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: ry5r6tel94UwBgDH9W5Fte7EIiv6bgckNtG5hqzNc3suTmIzKBaCjWQH3QW3OTET
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR15MB1898
X-Proofpoint-ORIG-GUID: MC0rCGygHAnDa7l9PqwRTX2V7VDEkloo
X-Proofpoint-GUID: MC0rCGygHAnDa7l9PqwRTX2V7VDEkloo
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.874,Hydra:6.0.486,FMLib:17.11.64.514
 definitions=2022-05-20_08,2022-05-20_02,2022-02-23_01
X-Spam-Status: No, score=-4.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        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_NONE,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
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 5/20/22 4:50 PM, Yonghong Song wrote:
> 
> 
> On 5/20/22 4:37 AM, Shung-Hsi Yu wrote:
>> The BPF_SIZE check in the beginning of check_ld_imm() actually guard
>> against program with JMP instructions that goes to the second
>> instruction of BPF_LD_IMM64, but may be easily dismissed as an simple
>> opcode check that's duplicating the effort of bpf_opcode_in_insntable().
>>
>> Add comment to better reflect the importance of the check.
>>
>> Signed-off-by: Shung-Hsi Yu <shung-hsi.yu@suse.com>
>> ---
>>   kernel/bpf/verifier.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
>> index 79a2695ee2e2..133929751f80 100644
>> --- a/kernel/bpf/verifier.c
>> +++ b/kernel/bpf/verifier.c
>> @@ -9921,6 +9921,10 @@ static int check_ld_imm(struct bpf_verifier_env 
>> *env, struct bpf_insn *insn)
>>       struct bpf_map *map;
>>       int err;
>> +    /* checks that this is not the second part of BPF_LD_IMM64, which is
>> +     * skipped over during opcode check, but a JMP with invalid 
>> offset may
>> +     * cause check_ld_imm() to be called upon it.
>> +     */
> 
> The check_ld_imm() call context is:
> 
>                  } else if (class == BPF_LD) {
>                          u8 mode = BPF_MODE(insn->code);
> 
>                          if (mode == BPF_ABS || mode == BPF_IND) {
>                                  err = check_ld_abs(env, insn);
>                                  if (err)
>                                          return err;
> 
>                          } else if (mode == BPF_IMM) {
>                                  err = check_ld_imm(env, insn);
>                                  if (err)
>                                          return err;
> 
>                                  env->insn_idx++;
>                                  sanitize_mark_insn_seen(env);
>                          } else {
>                                  verbose(env, "invalid BPF_LD mode\n");
>                                  return -EINVAL;
>                          }
>                  }
> 
> which is a normal checking of LD_imm64 insn.
> 
> I think the to-be-added comment is incorrect and unnecessary.

Okay, double check again and now I understand what happens
when hitting the second insn of ldimm64 with a branch target.
Here we have BPF_LD = 0 and BPF_IMM = 0, so for a branch
target to the 2nd part of ldimm64, it will come to
check_ld_imm() and have error "invalid BPF_LD_IMM insn"

So check_ld_imm() is to check whether the insn is a
*legal* insn for the first part of ldimm64.

So the comment may be rewritten as below.

This is to verify whether an insn is a BPF_LD_IMM64
or not. But since BPF_LD = 0 and BPF_IMM = 0, if the branch
target comes to the second part of BPF_LD_IMM64,
the control may come here as well.

> 
>>       if (BPF_SIZE(insn->code) != BPF_DW) {
>>           verbose(env, "invalid BPF_LD_IMM insn\n");
>>           return -EINVAL;