Message-ID: <ff77b4e7-49b5-61d8-12f1-b5927eb671a6@fb.com>
Date:   Sat, 8 Jan 2022 10:53:49 -0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.4.1
Subject: Re: [PATCH] bpf: btf: Fix a var size check in validator
Content-Language: en-US
To:     "Yichun Zhang (agentzh)" <yichun@openresty.com>
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>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        <netdev@vger.kernel.org>, <bpf@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <llvm@lists.linux.dev>
References: <20220108022212.962-1-yichun@openresty.com>
From:   Yonghong Song <yhs@fb.com>
In-Reply-To: <20220108022212.962-1-yichun@openresty.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?UVFpY1FPdXZ2UWN3THBpdUUrSEw1LzB1cklvRUVqV2pXTDJpZmRBbm1iWHpL?=
 =?utf-8?B?dGtINnN5SUN1dFNEZ0VibzFYWExmYk5MUi95TkNSR2FydUZXSlpYNEtBOEgx?=
 =?utf-8?B?c0tScDl1Yy9FejZBZkJIazhPcm9reGFzaWxCcGFHM3hoZ0NLRVJRL2lwUTdI?=
 =?utf-8?B?R2RWeXZLL0d0WFAxKzc0d3ZLODI1MHlxd1ZRWmd1VzVQM0JjVk1iWmxIUXVu?=
 =?utf-8?B?R2s3dHc5VU9JcHA4Zko3b2ZCM0V2MnF6cTlIZHFBSzViU3dwR1F6KzkxMHRk?=
 =?utf-8?B?bEpTRHRuSjR2MVVyMWJLMTNHa0FZWGEydDVnMlIyVktRWUhSQWV2ditwbEtB?=
 =?utf-8?B?VkZZNVRLa2hZcTlpRWg1NndjTFJ0eUxpS0t0OW9YdFZtK3pxUFNXNFNqVmRs?=
 =?utf-8?B?U1VGR2xQejBJRVhYeVNueVEyaEdGb1dyT0hUTm16MGF5WDJHSk8va0gvTXlN?=
 =?utf-8?B?bTFkdjBsaFZvUzhlS0VVa2hNTVkwRzRydkJ5Y0Y3bCtOUmJsQUxEeThUdW03?=
 =?utf-8?B?SmF6MWdyTkFjQkwya0w0Ly9MS1JuNk1PeDErZXFST3dRSVZPVURaUm9aZ3My?=
 =?utf-8?B?M0ZsRVV1WkpwREFXbXhwUFRobUtoR1E4MDVQL2NyZVBvSEEzRHpQU0Yvcnoy?=
 =?utf-8?B?ZEkzdUJpeUhwaENETlRWYjNsZWJzVWtIamxQYzVpUUdLVHdlbWEzQ0xqUHBq?=
 =?utf-8?B?ajB3dGppTzJqZ2hqTWwzMllVK1IvSVVCUkMwQ0JUOWhIaDRpbGltNkdta1hJ?=
 =?utf-8?B?bGF1OGVxendNd2hVSDVxeDRNVXZGVEh6ZHpiSmMzNiszSUdsdmZVMFFBckda?=
 =?utf-8?B?Znd2Y0UzM3RNSDMrVW5QMnQ1U3Fzd0YxSTdwQS8vM0NRRm5QbnlIZVhaeW4r?=
 =?utf-8?B?TlNETDh1M0RwWnRwQnh4a1QwdS9uemMzRHRVdU5MSmVkazI1cDJsdFdaUnlR?=
 =?utf-8?B?cGY1Kzkxc3FYVm1RbTRtZ2RIdW84Qmd4VUpEUmJWSU5CcnlXSkpHSWFUWUpo?=
 =?utf-8?B?NHRlL3RnWG5KQTJzaDFJOEQ4QzkrNGgxaVBFTlN6SjRmUkgxbDFxTjl3Q2Ux?=
 =?utf-8?B?dlRDTFh1alJRQ1cwL25nSTgwKzlobnZ1RE1CbTV0SW0vUTBqQmtWNVh1dnZu?=
 =?utf-8?B?WFdCRXlUUVZxcWxudjZkV3VxSUU1SHZ1SVY4cDFkanEwM0dqeTBmaTlHMWU3?=
 =?utf-8?B?QytmeEhRcVhGOG9PT1g0aHpjNFgvV0syL1VHQmdFQlVpS2ZhQkZlTWQ5UHJn?=
 =?utf-8?B?VGROWFJzSTJjNDE2UnoxVGJzNUZlWVE0bnJQeENSNFdXWHltNFFHQ3M3d1c2?=
 =?utf-8?B?bGlHOVhvL3YrSnlrakIvbVEycmZUcHNKbTRGSGorSk9iMFhwZlp5WnJmKzNF?=
 =?utf-8?B?ZkorWFBqZStkZC9GUGdTWFZicnJiUUU5U0UrY05pUmExd3BRbHRPanF5SWMw?=
 =?utf-8?B?K0NxbXVSZnQ5dHBYdVpMOTNleVVaNFp2a3pWN09XaFZsUnBXcE0ySWVwRDJx?=
 =?utf-8?B?YkRCQkRkcENFMEpQMS96emhnQlovQVZVSldMbGhTMVlzOVZoYjN6T29INFVV?=
 =?utf-8?B?ZHhpcFY3Zm01aStUWE0wT3VoNHU3aXVmKzUxd3lWRnVSKzR3ak16ZzJUZFZ0?=
 =?utf-8?B?MkliRDNHWTNRTEpiRUEwK1k1SDBiSHh4eFk0aXgvOG5NTkt2OFhXMS9mNGZK?=
 =?utf-8?B?TmNlWGVtYTNIVW9TN3hmeEVIVi9sdTljM0FUZzUveUY0M0JmWCtPNFQvYk1C?=
 =?utf-8?B?QklTQ2ZwUlFiOGFTcU5iSUJQektpdHFqUUt3TFNPVVkrOElabmp6WE0rNzc0?=
 =?utf-8?B?ZGdXMFhranpaUHJPMTNDYm5vdlV5WXVSK0pVMjlmNTd3TEgxaE5vNEdUK3Za?=
 =?utf-8?B?VUtHejIzM0VvRzNGZ2Y3dTNFUmZ0eXZyejhhVCtQZGwxUnUyYzQ2K3VCR3U4?=
 =?utf-8?B?Qk5QajlJa3U4UnJVRDB1QlBNRWM1SmZvM2tkZGF2Y1VGTFMyczVHcW1EOENQ?=
 =?utf-8?B?Tnk4ZVNQZ3hZdFR2YmRMR3I1Mm1lV1JTV2NzVlZBU1Q4ckwyL2c1Z1dMU09L?=
 =?utf-8?B?SU9wZDIwb3Q0SGxBYXhoMzk0RXFjZDJhOEVJMjd4VGRFSWU5TUQ5MUZUV0lv?=
 =?utf-8?Q?X6UbtpEWi/cOx7tKhjXQzCyl2?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 5dea2bf9-5983-4221-bd52-08d9d2d8376b
X-MS-Exchange-CrossTenant-AuthSource: SN6PR1501MB2064.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Jan 2022 18:53:53.6868
 (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: 7B20lmSTJULkEolJD4ppEf/5kDDHbsS62x2WeqAmSKyIbqE5Hn7EZHftRfi4yGQX
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR15MB4224
X-OriginatorOrg: fb.com
X-Proofpoint-GUID: NbI48dCtOMdW11qwbmPu2jR7sCQqHk94
X-Proofpoint-ORIG-GUID: NbI48dCtOMdW11qwbmPu2jR7sCQqHk94
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.790,Hydra:6.0.425,FMLib:17.11.62.513
 definitions=2022-01-08_07,2022-01-07_01,2021-12-02_01
X-Proofpoint-Spam-Details: rule=fb_outbound_notspam policy=fb_outbound score=0 malwarescore=0
 phishscore=0 clxscore=1011 priorityscore=1501 mlxscore=0 bulkscore=0
 lowpriorityscore=0 suspectscore=0 spamscore=0 mlxlogscore=999
 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx
 scancount=1 engine=8.12.0-2110150000 definitions=main-2201080143
X-FB-Internal: deliver
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 1/7/22 6:22 PM, Yichun Zhang (agentzh) wrote:
> The btf validator should croak when the variable size is larger than
> its type size, not less. The LLVM optimizer may use smaller sizes for
> the C type.
> 
> We ran into this issue with real-world BPF programs emitted by the
> latest version of Clang/LLVM.

Could you give an example to show we have variable size is less than 
type_size?

> 
> Fixes: 1dc92851849cc ("bpf: kernel side support for BTF Var and DataSec")
> Signed-off-by: Yichun Zhang (agentzh) <yichun@openresty.com>
> ---
>   kernel/bpf/btf.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 9bdb03767db5..2a6967b13ce1 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -3696,7 +3696,7 @@ static int btf_datasec_resolve(struct btf_verifier_env *env,
>   			return -EINVAL;
>   		}
>   
> -		if (vsi->size < type_size) {
> +		if (vsi->size > type_size) {
>   			btf_verifier_log_vsi(env, v->t, vsi, "Invalid size");
>   			return -EINVAL;
>   		}

The fix is incorrect. We do have cases where variable size greater than 
type_size. For example,

$ cat t.c
struct t {
   int  g;
   char a[];
} v = {1, {'a', 'b', 'c', 0}};

The type (struct t) size is 4, but the variable size is 8.