From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pj1-x102d.google.com (mail-pj1-x102d.google.com [IPv6:2607:f8b0:4864:20::102d]) by sourceware.org (Postfix) with ESMTPS id 85C4E3858D1E for ; Mon, 29 Jan 2024 17:25:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 85C4E3858D1E Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=chromium.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 85C4E3858D1E Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::102d ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1706549109; cv=none; b=aNZXiMiAnRHRwWslBHjibi9OHor7v9jQCD0tTKRMmIez5FqD13d/X6GWHN4+i38uvmfQdvv2BAZOrzAPfweeQRdTBb3E51GWJZ+kBFXAO6C4Ngx34VAU0ZZXH1VAQzM1+wsJ/Ia6kuNiC0OlrtZCim04EknFyEGMmOHJDgO3zRI= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1706549109; c=relaxed/simple; bh=jII2MSCADnxz0FISttfFTW/Nd3ES4yJOGqyjYx/lEHY=; h=DKIM-Signature:Date:From:To:Subject:Message-ID:MIME-Version; b=kd3FBXdYctjABPjPBt4fxPxv5y4EjQg9whUDHioy7aTTKSC5aedQeQEOfeMfB+leFTrCZrORrkdsnZYtASSp9DoGB5Po8qFAgE6aV3pbGhZ5QGfPYv97JtsWy1LDlS6hdxug/Dr+3tzRrXVnmqoMaBlCzWcnNpXNB5/9eVl7JHk= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-pj1-x102d.google.com with SMTP id 98e67ed59e1d1-290b9f83037so1942594a91.1 for ; Mon, 29 Jan 2024 09:25:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; t=1706549106; x=1707153906; darn=gcc.gnu.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date:from:to :cc:subject:date:message-id:reply-to; bh=G0lfo1zRu6pOnDIDcA280PDEhOaYdcXR20nHgXru2dU=; b=Tvtqb0ZPpT/ddqMoISt7noKjjdJ6nvzzPNZJTHHEjN+OPYcsNjreTBqftal45h4Gqx drdLucG1eV3fByWRbiNlXcfc5/f6tQXUmuXq++z9mKgXw78z1A9/DKK40kW8rJVGJU5L IFQT61yy7TRlqw0EE9YHJFtYQnbpYKJUBPSfY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1706549106; x=1707153906; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:from:date :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=G0lfo1zRu6pOnDIDcA280PDEhOaYdcXR20nHgXru2dU=; b=XYSFYfrBeC++c8oDHVLZVCFVlwQcarQ9ndO9SkQVg4ql9KHyETuofBKe2F9hJYiMd1 Bh13jz0oxMTWCkK1QLUjaquVZX8QAiUGFKaaST0tlxVLeY+aT+JVWpiOKLim/xchLg6b g1qK25OS0VJwx4GxG0fe8FEgVaZEKDxbPK2AhqszeP0asK3ZcM25rP6MLIv6Pa3v9O98 sx34CYrc+XnJpa5cKN8hydBjuq3O/ZqOYR0jKXFK3KPl0dP3xUVwIynVj7pgE7NWUd9F /RQhtF2oq5OgR+FZGtCa3fwwo/a/mnXrhBeW+C1Ek7s2xT1zmf8wo5ME8NfygSOAGN8J dgtg== X-Gm-Message-State: AOJu0YxsmSr0Io4547hP6N1O62CIvwCWWgUYOpsudolrT6e6YfgOI82D /7MV9f4YYTYJvdVEsRJscO+ZQc6Jien/cCtok0SCvi1sBDaxjiFSBdh9uIG+aQqkJ4a1HB67LSQ = X-Google-Smtp-Source: AGHT+IG9D5tSofDS/vxbDFpd4sX1DGprNd1YcjTRT2GGGjhT1FAaakgFe55GX2F+sgzhDgPOsrYxeg== X-Received: by 2002:a17:90b:19d5:b0:290:2af9:55eb with SMTP id nm21-20020a17090b19d500b002902af955ebmr5664848pjb.22.1706549106072; Mon, 29 Jan 2024 09:25:06 -0800 (PST) Received: from www.outflux.net ([198.0.35.241]) by smtp.gmail.com with ESMTPSA id e9-20020a17090a77c900b0029487e5dbc6sm5773600pjs.31.2024.01.29.09.25.05 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 29 Jan 2024 09:25:05 -0800 (PST) Date: Mon, 29 Jan 2024 09:25:04 -0800 From: Kees Cook To: Qing Zhao Cc: Martin Uecker , "josmyers@redhat.com" , Richard Biener , "jakub@redhat.com" , "siddhesh@gotplt.org" , "isanbard@gmail.com" , "gcc-patches@gcc.gnu.org" Subject: Re: [PATCH v4 0/4]New attribute "counted_by" to annotate bounds for C99 FAM(PR108896) Message-ID: <202401290911.FEF85CC6@keescook> References: <20240124002955.3387096-1-qing.zhao@oracle.com> <202401241624.4DC3D829@keescook> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Spam-Status: No, score=-2.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,JMQ_SPF_NEUTRAL,RCVD_IN_DNSWL_NONE,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 Mon, Jan 29, 2024 at 04:00:20PM +0000, Qing Zhao wrote: > An update on the kernel building with my version 4 patch. > > Kees reported two FE issues with the current version 4 patch: > > 1. The operator “typeof” cannot return correct type for a->array; > 2. The operator “&” cannot return correct address for a->array; > > I fixed both in my local repository. > > With these additional fix. Kernel with counted-by annotation can be built successfully. Thanks for the fixes! > > And then, Kees reported one behavioral issue with the current counted-by: > > When the counted-by value is below zero, my current patch > > A. Didn’t report any warning for it. > B. Accepted the negative value as a wrapped size. > > i.e. for: > > struct foo { > signed char size; > unsigned char array[] __counted_by(size); > } *a; > > ... > a->size = -3; > report(__builtin_dynamic_object_size(p->array, 1)); > > this reports 253, rather than 0. > > And the array-bounds sanitizer doesn’t catch negative index bounds neither. > > a->size = -3; > report(a->array[1]); // does not trap > > > So, my questions are: > > How should we handle the negative counted-by value? Treat it as always 0-bounded: count < 0 ? 0 : count > > My approach is: > > I think that this is a user error, the compiler need to Issue warning during runtime about this user error. > > Since I have one remaining patch that has not been finished yet: > > 6 Emit warnings when the user breaks the requirments for the new counted_by attribute > compilation time: -Wcounted-by > run time: -fsanitizer=counted-by > * The initialization to the size field should be done before the first reference to the FAM field. I would hope that regular compile-time warnings would catch this. > * the array has at least # of elements specified by the size field all the time during the program. > * the value of counted-by should not be negative. This seems reasonable for a very strict program, but it won't work for the kernel as-is: a negative "count" is sometimes used to carry failure details back to other users of the structure. This could be refactored in the kernel, but I'd prefer that even without -fsanitizer=counted-by the runtime behaviors will be "safe". It does not seem sensible to me that adding a buffer size validation primitive to GCC will result in conditions where a size calculation will wrap around. I prefer no surprises. :) > Let me know your comment and suggestions. Clang has implemented the safety logic I'd prefer: * __bdos will report 0 for any sizing where the "counted_by" count variable is negative. Effectively, the count variable is always processed as: count < 0 ? 0 : count struct foo { int count; short array[] __counted_by(count); } *p; __bdos(p->array, 1) ==> sizeof(*p->array) * (count < 0 ? 0 : count) The logic for this is that __bdos can be _certain_ that the size is 0 when the count variable is pathological. * -fsanitize=array-bounds similarly treats count as above, so that: printf("%d\n", p->array[index]); ==> trap when index > (count < 0 ? 0 : count) Same logic for the sanitizer: any access to the array when count is invalid means the access is invalid and must be trapped. This means that software can run safely even in pathological conditions. -Kees -- Kees Cook