From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from us-smtp-delivery-74.mimecast.com (us-smtp-delivery-74.mimecast.com [170.10.129.74]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id C9543C433F5 for ; Thu, 5 May 2022 22:43:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1651790619; h=from:from:sender:sender: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:list-id:list-help: list-unsubscribe:list-subscribe:list-post; bh=g5tfMwg0Wkrnl+sQiJ01eJvZ4ZVsJqsa1c429cc9biU=; b=OfTMBkyW8dxdNFGNcLv3r15x0ff2WCb3df9PDXCTOsGm8ehFuhK44ilhdLLSBdS/MUI3VM Xru0Vg2tDKztIu9N6NjZOhNEBjDhHH4FDDe9N3VdA0IErZXVxTLmN2g0kDBDzJPAcbWDgU 8SDIzrRKXXKhzt0vvW41g2Uxou6vX9A= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-493-r_Z8YeGwPL-6PESCJ1Ga_w-1; Thu, 05 May 2022 18:43:36 -0400 X-MC-Unique: r_Z8YeGwPL-6PESCJ1Ga_w-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id EE8D6811E76; Thu, 5 May 2022 22:43:34 +0000 (UTC) Received: from mm-prod-listman-01.mail-001.prod.us-east-1.aws.redhat.com (unknown [10.30.29.100]) by smtp.corp.redhat.com (Postfix) with ESMTP id 019012166B17; Thu, 5 May 2022 22:43:33 +0000 (UTC) Received: from mm-prod-listman-01.mail-001.prod.us-east-1.aws.redhat.com (localhost [IPv6:::1]) by mm-prod-listman-01.mail-001.prod.us-east-1.aws.redhat.com (Postfix) with ESMTP id C981A1947048; Thu, 5 May 2022 22:43:32 +0000 (UTC) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) by mm-prod-listman-01.mail-001.prod.us-east-1.aws.redhat.com (Postfix) with ESMTP id 3D0801947047 for ; Thu, 5 May 2022 22:43:31 +0000 (UTC) Received: by smtp.corp.redhat.com (Postfix) id 27729400F75F; Thu, 5 May 2022 22:43:31 +0000 (UTC) Received: from x2.localnet (unknown [10.22.10.83]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 3E3F540CF900; Thu, 5 May 2022 22:43:30 +0000 (UTC) From: Steve Grubb To: Richard Guy Briggs , Jan Kara Subject: Re: [PATCH v2 2/3] fanotify: define struct members to hold response decision context Date: Thu, 05 May 2022 18:43:29 -0400 Message-ID: <3488909.R56niFO833@x2> Organization: Red Hat In-Reply-To: <20220505144456.nw6slyqw4pjizl5p@quack3.lan> References: <20220505144456.nw6slyqw4pjizl5p@quack3.lan> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.11.54.1 X-BeenThere: linux-audit@redhat.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux Audit Discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Jan Kara , Amir Goldstein , LKML , Linux-Audit Mailing List , linux-fsdevel@vger.kernel.org, Eric Paris Errors-To: linux-audit-bounces@redhat.com Sender: "Linux-audit" X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=linux-audit-bounces@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Hello Jan, On Thursday, May 5, 2022 10:44:56 AM EDT Jan Kara wrote: > On Tue 03-05-22 21:33:35, Richard Guy Briggs wrote: > > On 2022-05-02 20:16, Paul Moore wrote: > > > On Thu, Apr 28, 2022 at 8:45 PM Richard Guy Briggs wrote: > > > > This patch adds 2 structure members to the response returned from > > > > user > > > > space on a permission event. The first field is 16 bits for the > > > > context > > > > type. The context type will describe what the meaning is of the > > > > second > > > > field. The default is none. The patch defines one additional context > > > > type which means that the second field is a 32-bit rule number. This > > > > will allow for the creation of other context types in the future if > > > > other users of the API identify different needs. The second field > > > > size > > > > is defined by the context type and can be used to pass along the data > > > > described by the context. > > > > > > > > To support this, there is a macro for user space to check that the > > > > data > > > > being sent is valid. Of course, without this check, anything that > > > > overflows the bit field will trigger an EINVAL based on the use of > > > > FAN_INVALID_RESPONSE_MASK in process_access_response(). > > ... > > > > > static ssize_t fanotify_write(struct file *file, const char __user > > > > *buf, size_t count, loff_t *pos) { > > > > > > > > - struct fanotify_response response = { .fd = -1, .response = > > > > -1 }; > > > > + struct fanotify_response response; > > > > > > > > struct fsnotify_group *group; > > > > int ret; > > > > > > > > + size_t size = min(count, sizeof(struct fanotify_response)); > > > > > > > > if (!IS_ENABLED(CONFIG_FANOTIFY_ACCESS_PERMISSIONS)) > > > > > > > > return -EINVAL; > > > > > > > > group = file->private_data; > > > > > > > > - if (count < sizeof(response)) > > > > + if (count < offsetof(struct fanotify_response, > > > > extra_info_buf)) > > > > > > > > return -EINVAL; > > > > > > Is this why you decided to shrink the fanotify_response:response field > > > from 32-bits to 16-bits? I hope not. I would suggest both keeping > > > the existing response field as 32-bits and explicitly checking for > > > writes that are either the existing/compat length as well as the > > > newer, longer length. > > > > No. I shrank it at Jan's suggestion. I think I agree with you that > > the response field should be kept at u32 as it is defined in userspace > > and purge the doubt about what would happen with a new userspace with > > an old kernel. > > Hum, for the life of me I cannot find my response you mention here. Can you > send a link so that I can refresh my memory? It has been a long time... It was this thread: https://marc.info/?t=160148236400005&r=1&w=2 -Steve > > > > + > > > > +#define FANOTIFY_RESPONSE_EXTRA_LEN_MAX \ > > > > + (sizeof(union { \ > > > > + struct fanotify_response_audit_rule r; \ > > > > + /* add other extra info structures here */ \ > > > > + })) > > > > + > > > > > > > > struct fanotify_response { > > > > > > > > __s32 fd; > > > > > > > > - __u32 response; > > > > + __u16 response; > > > > + __u16 extra_info_type; > > > > + char extra_info_buf[FANOTIFY_RESPONSE_EXTRA_LEN_MAX]; > > > > > > > > }; > > > > > > Since both the kernel and userspace are going to need to agree on the > > > content and formatting of the fanotify_response:extra_info_buf field, > > > why is it hidden behind a char array? You might as well get rid of > > > that abstraction and put the union directly in the fanotify_response > > > struct. It is possible you could also get rid of the > > > fanotify_response_audit_rule struct this way too and just access the > > > rule scalar directly. > > > > This does make sense and my only concern would be a variable-length > > type. There isn't any reason to hide it. If userspace chooses to use > > the old interface and omit the type field then it defaults to NONE. > > > > If future types with variable data are defined, the first field could be > > a u32 that unions with the rule number that won't change the struct > > size. > > Struct fanotify_response size must not change, it is part of the kernel > ABI. In particular your above change would break userspace code that is > currently working just fine (e.g. allocating 8 bytes and expecting struct > fanotify_response fits there, or just writing sizeof(struct > fanotify_response) as a response while initializing only first 8 bytes). > How I'd suggest doing it now (and I'd like to refresh my memory from my > past emails you mention because in the past I might have thought something > else ;)) is that you add another flag to 'response' field similar to > FAN_AUDIT - like FAN_EXTRA_INFO. If that is present, it means extra info is > to be expected after struct fanotify_response. The extra info would always > start with a header like: > > struct fanotify_response_info_header { > __u8 info_type; > __u8 pad; > __u16 len; /* This is including the header itself */ > } > > And after such header there would be the 'blob' of data 'len - header size' > long. We use this same scheme when passing fanotify events to userspace > and it has proven to be lightweight and extensible. It covers the > situation when in the future audit would decide it wants other data (just > change data type), it would also cover the situation when some other > subsystem wants its information passed as well - there can be more > structures like this attached at the end, we can process the response up > to the length of the write. > > Now these are just possible future extensions making sure we can extend the > ABI without too much pain. In the current implementation I'd just return > EINVAL whenever more than FANOTIFY_RESPONSE_MAX_LEN (16 bytes) is written > and do very strict checks on what gets passed in. It is also trivially > backwards compatible (old userspace on new kernel works just fine). > > If you want to achieve compatibility of running new userspace on old kernel > (I guess that's desirable), we have group flags for that - like we > introduced FAN_ENABLE_AUDIT to allow for FAN_AUDIT flag in response we now > need to add a flag like FAN_EXTENDED_PERMISSION_INFO telling the kernel it > should expect an allow more info returning for permission events. At the > same time this is the way for userspace to be able to tell whether the > kernel supports this. I know this sounds tedious but that's the cost of > extending the ABI in the compatible way. We've made various API mistakes > in the past having to add weird workarounds to fanotify and we don't want > to repeat those mistakes :). > > One open question I have is what should the kernel do with 'info_type' in > response it does not understand (in the future when there are possibly more > different info types). It could just skip it because this should be just > additional info for introspection (the only mandatory part is in > fanotify_response, however it could surprise userspace that passed info is > just getting ignored. To solve this we would have to somewhere report > supported info types (maybe in fanotify fdinfo in proc). I guess we'll > cross that bridge when we get to it. > > Amir, what do you think? > > Honza -- Linux-audit mailing list Linux-audit@redhat.com https://listman.redhat.com/mailman/listinfo/linux-audit