Linux-audit Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 2/3] fanotify: define bit map fields to hold response decision context
@ 2020-09-30 16:12 Steve Grubb
  2020-10-01 10:12 ` Jan Kara
  0 siblings, 1 reply; 4+ messages in thread
From: Steve Grubb @ 2020-09-30 16:12 UTC (permalink / raw)
  To: linux-fsdevel, Jan Kara, linux-audit, Paul Moore; +Cc: Amir Goldstein

This patch defines 2 bit maps within the response variable returned from
user space on a permission event. The first field is 3 bits for the context
type. The context type will describe what the meaning is of the second bit
field. The default is none. The patch defines one additional context type
which means that the second field is a rule number. This will allow for the
creation of 6 other context types in the future if other users of the API
identify different needs. The second field is 10 bits wide and can be used
to pass along the data described by the context. Neither of these bit maps
are directly adjacent and could be expanded if the need arises.

To support this, several macros were created to facilitate storing and
retrieving the values. There is also 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
of INVALID_RESPONSE_MASK in process_access_response().

Signed-off-by: Steve Grubb <sgrubb@redhat.com>
---
 fs/notify/fanotify/fanotify.c      |  3 +--
 fs/notify/fanotify/fanotify_user.c |  7 +------
 include/linux/fanotify.h           |  5 +++++
 include/uapi/linux/fanotify.h      | 31 ++++++++++++++++++++++++++++++
 4 files changed, 38 insertions(+), 8 deletions(-)

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 85eda539b35f..e72b7e59aa24 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -178,11 +178,10 @@ static int fanotify_get_response(struct fsnotify_group *group,
 	}
 
 	/* userspace responded, convert to something usable */
-	switch (event->response & ~FAN_AUDIT) {
+	switch (FAN_DEC_MASK(event->response)) {
 	case FAN_ALLOW:
 		ret = 0;
 		break;
-	case FAN_DENY:
 	default:
 		ret = -EPERM;
 	}
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index c8da9ea1e76e..3b8e515904fc 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -187,13 +187,8 @@ static int process_access_response(struct fsnotify_group *group,
 	 * userspace can send a valid response or we will clean it up after the
 	 * timeout
 	 */
-	switch (response & ~FAN_AUDIT) {
-	case FAN_ALLOW:
-	case FAN_DENY:
-		break;
-	default:
+	if (FAN_INVALID_RESPONSE_MASK(response))
 		return -EINVAL;
-	}
 
 	if (fd < 0)
 		return -EINVAL;
diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
index b79fa9bb7359..b3281d0e1b55 100644
--- a/include/linux/fanotify.h
+++ b/include/linux/fanotify.h
@@ -72,6 +72,11 @@
 #define ALL_FANOTIFY_EVENT_BITS		(FANOTIFY_OUTGOING_EVENTS | \
 					 FANOTIFY_EVENT_FLAGS)
 
+/* This mask is to check for invalid bits of a user space permission response */
+#define FAN_INVALID_RESPONSE_MASK(x) ((x) & ~(FAN_ALLOW | FAN_DENY | \
+					FAN_AUDIT | FAN_DEC_CONTEXT_TYPE | \
+					FAN_DEC_CONTEXT))
+
 /* Do not use these old uapi constants internally */
 #undef FAN_ALL_CLASS_BITS
 #undef FAN_ALL_INIT_FLAGS
diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
index a88c7c6d0692..785d68ebcb58 100644
--- a/include/uapi/linux/fanotify.h
+++ b/include/uapi/linux/fanotify.h
@@ -152,6 +152,37 @@ struct fanotify_response {
 #define FAN_DENY	0x02
 #define FAN_AUDIT	0x10	/* Bit mask to create audit record for result */
 
+/*
+ * User space may need to record additional information about its decision.
+ * The context type records what kind of information is included. A bit mask
+ * defines the type of information included and then the context of the
+ * decision. The context type is 3 bits allowing for 8 kinds of context.
+ * The default is none. We also define 10 bits to allow up to 1024 kinds of
+ * context to be returned.
+ *
+ * If the context type is Rule, then the context following is the rule number
+ * that triggered the user space decision.
+ *
+ * There are helper macros defined so that it can be standardized across tools.
+ * A full example of how user space can use this looks like this:
+ *
+ * if (FAN_DEC_CONTEXT_VALUE_VALID(rule_number))
+ *	response.response = FAN_DENY | FAN_AUDIT | FAN_DEC_CONTEXT_TYPE_RULE |
+ *			    FAN_DEC_CONTEXT_VALUE(rule_number);
+ */
+#define FAN_DEC_MASK(x) ((x) & (FAN_ALLOW|FAN_DENY))
+#define FAN_DEC_CONTEXT_TYPE 0x70000000
+#define FAN_DEC_CONTEXT      0x00FFC000
+
+#define FAN_DEC_CONTEXT_TYPE_VALUE(x)    (((x) & 0x07) << 28)
+#define FAN_DEC_CONTEXT_TYPE_TO_VALUE(x) (((x) & FAN_DEC_CONTEXT_TYPE) >> 28)
+#define FAN_DEC_CONTEXT_VALUE(x)         (((x) & 0x3FF) << 14)
+#define FAN_DEC_CONTEXT_TO_VALUE(x)      (((x) & FAN_DEC_CONTEXT) >> 14)
+#define FAN_DEC_CONTEXT_VALUE_VALID(x)   ((x) >= 0 && (x) < 1024)
+
+#define FAN_DEC_CONTEXT_TYPE_NONE  FAN_DEC_CONTEXT_TYPE_VALUE(0)
+#define FAN_DEC_CONTEXT_TYPE_RULE  FAN_DEC_CONTEXT_TYPE_VALUE(1)
+
 /* No fd set in event */
 #define FAN_NOFD	-1
 
-- 
2.26.2




--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 2/3] fanotify: define bit map fields to hold response decision  context
  2020-09-30 16:12 [PATCH 2/3] fanotify: define bit map fields to hold response decision context Steve Grubb
@ 2020-10-01 10:12 ` Jan Kara
  2020-10-01 10:24   ` Jan Kara
  0 siblings, 1 reply; 4+ messages in thread
From: Jan Kara @ 2020-10-01 10:12 UTC (permalink / raw)
  To: Steve Grubb; +Cc: Jan Kara, Amir Goldstein, linux-audit, linux-fsdevel

On Wed 30-09-20 12:12:28, Steve Grubb wrote:
> This patch defines 2 bit maps within the response variable returned from
> user space on a permission event. The first field is 3 bits for the context
> type. The context type will describe what the meaning is of the second bit
> field. The default is none. The patch defines one additional context type
> which means that the second field is a rule number. This will allow for the
> creation of 6 other context types in the future if other users of the API
> identify different needs. The second field is 10 bits wide and can be used
> to pass along the data described by the context. Neither of these bit maps
> are directly adjacent and could be expanded if the need arises.
> 
> To support this, several macros were created to facilitate storing and
> retrieving the values. There is also 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
> of INVALID_RESPONSE_MASK in process_access_response().
> 
> Signed-off-by: Steve Grubb <sgrubb@redhat.com>

So how likely do you find other context types are or that you'd need to
further expand the information passed from userspace? Because if there are
decent changes the expansion will be needed then I'd rather do something
like:

struct fanotify_response {
	__s32 fd;
	__u16 response;
	__u16 extra_info_type;
};

which is also backwards compatible and then userspace can set extra_info_type
and based on the type we'd know how much more bytes we need to copy from
buf to get additional information for that info type. 

This is much more flexible than what you propose and not that complex to
implement, you get type safety for extra information and don't need to use
macros to cram everything in u32 etc. Overall cleaner interface I'd say.

So in your case you'd then have something like:

#define FAN_RESPONSE_INFO_AUDIT_RULE 1

struct fanotify_response_audit_rule {
	__u32 rule;
};

								Honza

> ---
>  fs/notify/fanotify/fanotify.c      |  3 +--
>  fs/notify/fanotify/fanotify_user.c |  7 +------
>  include/linux/fanotify.h           |  5 +++++
>  include/uapi/linux/fanotify.h      | 31 ++++++++++++++++++++++++++++++
>  4 files changed, 38 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 85eda539b35f..e72b7e59aa24 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -178,11 +178,10 @@ static int fanotify_get_response(struct fsnotify_group *group,
>  	}
>  
>  	/* userspace responded, convert to something usable */
> -	switch (event->response & ~FAN_AUDIT) {
> +	switch (FAN_DEC_MASK(event->response)) {
>  	case FAN_ALLOW:
>  		ret = 0;
>  		break;
> -	case FAN_DENY:
>  	default:
>  		ret = -EPERM;
>  	}
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index c8da9ea1e76e..3b8e515904fc 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -187,13 +187,8 @@ static int process_access_response(struct fsnotify_group *group,
>  	 * userspace can send a valid response or we will clean it up after the
>  	 * timeout
>  	 */
> -	switch (response & ~FAN_AUDIT) {
> -	case FAN_ALLOW:
> -	case FAN_DENY:
> -		break;
> -	default:
> +	if (FAN_INVALID_RESPONSE_MASK(response))
>  		return -EINVAL;
> -	}
>  
>  	if (fd < 0)
>  		return -EINVAL;
> diff --git a/include/linux/fanotify.h b/include/linux/fanotify.h
> index b79fa9bb7359..b3281d0e1b55 100644
> --- a/include/linux/fanotify.h
> +++ b/include/linux/fanotify.h
> @@ -72,6 +72,11 @@
>  #define ALL_FANOTIFY_EVENT_BITS		(FANOTIFY_OUTGOING_EVENTS | \
>  					 FANOTIFY_EVENT_FLAGS)
>  
> +/* This mask is to check for invalid bits of a user space permission response */
> +#define FAN_INVALID_RESPONSE_MASK(x) ((x) & ~(FAN_ALLOW | FAN_DENY | \
> +					FAN_AUDIT | FAN_DEC_CONTEXT_TYPE | \
> +					FAN_DEC_CONTEXT))
> +
>  /* Do not use these old uapi constants internally */
>  #undef FAN_ALL_CLASS_BITS
>  #undef FAN_ALL_INIT_FLAGS
> diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
> index a88c7c6d0692..785d68ebcb58 100644
> --- a/include/uapi/linux/fanotify.h
> +++ b/include/uapi/linux/fanotify.h
> @@ -152,6 +152,37 @@ struct fanotify_response {
>  #define FAN_DENY	0x02
>  #define FAN_AUDIT	0x10	/* Bit mask to create audit record for result */
>  
> +/*
> + * User space may need to record additional information about its decision.
> + * The context type records what kind of information is included. A bit mask
> + * defines the type of information included and then the context of the
> + * decision. The context type is 3 bits allowing for 8 kinds of context.
> + * The default is none. We also define 10 bits to allow up to 1024 kinds of
> + * context to be returned.
> + *
> + * If the context type is Rule, then the context following is the rule number
> + * that triggered the user space decision.
> + *
> + * There are helper macros defined so that it can be standardized across tools.
> + * A full example of how user space can use this looks like this:
> + *
> + * if (FAN_DEC_CONTEXT_VALUE_VALID(rule_number))
> + *	response.response = FAN_DENY | FAN_AUDIT | FAN_DEC_CONTEXT_TYPE_RULE |
> + *			    FAN_DEC_CONTEXT_VALUE(rule_number);
> + */
> +#define FAN_DEC_MASK(x) ((x) & (FAN_ALLOW|FAN_DENY))
> +#define FAN_DEC_CONTEXT_TYPE 0x70000000
> +#define FAN_DEC_CONTEXT      0x00FFC000
> +
> +#define FAN_DEC_CONTEXT_TYPE_VALUE(x)    (((x) & 0x07) << 28)
> +#define FAN_DEC_CONTEXT_TYPE_TO_VALUE(x) (((x) & FAN_DEC_CONTEXT_TYPE) >> 28)
> +#define FAN_DEC_CONTEXT_VALUE(x)         (((x) & 0x3FF) << 14)
> +#define FAN_DEC_CONTEXT_TO_VALUE(x)      (((x) & FAN_DEC_CONTEXT) >> 14)
> +#define FAN_DEC_CONTEXT_VALUE_VALID(x)   ((x) >= 0 && (x) < 1024)
> +
> +#define FAN_DEC_CONTEXT_TYPE_NONE  FAN_DEC_CONTEXT_TYPE_VALUE(0)
> +#define FAN_DEC_CONTEXT_TYPE_RULE  FAN_DEC_CONTEXT_TYPE_VALUE(1)
> +
>  /* No fd set in event */
>  #define FAN_NOFD	-1
>  
> -- 
> 2.26.2
> 
> 
> 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 2/3] fanotify: define bit map fields to hold response decision  context
  2020-10-01 10:12 ` Jan Kara
@ 2020-10-01 10:24   ` Jan Kara
  2020-10-01 20:33     ` Paul Moore
  0 siblings, 1 reply; 4+ messages in thread
From: Jan Kara @ 2020-10-01 10:24 UTC (permalink / raw)
  To: Steve Grubb; +Cc: Jan Kara, Amir Goldstein, linux-audit, linux-fsdevel

On Thu 01-10-20 12:12:19, Jan Kara wrote:
> On Wed 30-09-20 12:12:28, Steve Grubb wrote:
> > This patch defines 2 bit maps within the response variable returned from
> > user space on a permission event. The first field is 3 bits for the context
> > type. The context type will describe what the meaning is of the second bit
> > field. The default is none. The patch defines one additional context type
> > which means that the second field is a rule number. This will allow for the
> > creation of 6 other context types in the future if other users of the API
> > identify different needs. The second field is 10 bits wide and can be used
> > to pass along the data described by the context. Neither of these bit maps
> > are directly adjacent and could be expanded if the need arises.
> > 
> > To support this, several macros were created to facilitate storing and
> > retrieving the values. There is also 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
> > of INVALID_RESPONSE_MASK in process_access_response().
> > 
> > Signed-off-by: Steve Grubb <sgrubb@redhat.com>
> 
> So how likely do you find other context types are or that you'd need to
> further expand the information passed from userspace? Because if there are
> decent changes the expansion will be needed then I'd rather do something
> like:
> 
> struct fanotify_response {
> 	__s32 fd;
> 	__u16 response;
> 	__u16 extra_info_type;
> };
> 
> which is also backwards compatible and then userspace can set extra_info_type
> and based on the type we'd know how much more bytes we need to copy from
> buf to get additional information for that info type. 
> 
> This is much more flexible than what you propose and not that complex to
> implement, you get type safety for extra information and don't need to use
> macros to cram everything in u32 etc. Overall cleaner interface I'd say.
> 
> So in your case you'd then have something like:
> 
> #define FAN_RESPONSE_INFO_AUDIT_RULE 1
> 
> struct fanotify_response_audit_rule {
> 	__u32 rule;
> };

Now I realized we need to propagate the extra information through fanotify
permission event to audit - that can be also done relatively easily. Just
add '__u16 extra_info_type' to fanotify_perm_event and 'char
extra_info_buf[FANOTIFY_MAX_RESPONSE_EXTRA_LEN]' for now for the data. If
we ever grow larger extra info structures, we can always change this to
dynamic allocation but that will be only internal fanotify change,
userspace facing API can stay the same.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH 2/3] fanotify: define bit map fields to hold response decision context
  2020-10-01 10:24   ` Jan Kara
@ 2020-10-01 20:33     ` Paul Moore
  0 siblings, 0 replies; 4+ messages in thread
From: Paul Moore @ 2020-10-01 20:33 UTC (permalink / raw)
  To: Jan Kara, Steve Grubb; +Cc: linux-fsdevel, linux-audit, Amir Goldstein

On Thu, Oct 1, 2020 at 6:24 AM Jan Kara <jack@suse.cz> wrote:
> On Thu 01-10-20 12:12:19, Jan Kara wrote:
> > On Wed 30-09-20 12:12:28, Steve Grubb wrote:
> > > This patch defines 2 bit maps within the response variable returned from

Please use "bitmaps" instead of "bit maps".

> > So how likely do you find other context types are or that you'd need to
> > further expand the information passed from userspace? Because if there are
> > decent changes the expansion will be needed then I'd rather do something
> > like:
> >
> > struct fanotify_response {
> >       __s32 fd;
> >       __u16 response;
> >       __u16 extra_info_type;
> > };
> >
> > which is also backwards compatible and then userspace can set extra_info_type
> > and based on the type we'd know how much more bytes we need to copy from
> > buf to get additional information for that info type.
> >
> > This is much more flexible than what you propose and not that complex to
> > implement, you get type safety for extra information and don't need to use
> > macros to cram everything in u32 etc. Overall cleaner interface I'd say.

Yes, much cleaner.  Stealing bits from an integer is one of those
things you do as a last resort when you can't properly change an API.
Considering that APIs always tend to grow and hardly ever shrink, I
would much prefer Jan's suggestion.

> Now I realized we need to propagate the extra information through fanotify
> permission event to audit - that can be also done relatively easily. Just
> add '__u16 extra_info_type' to fanotify_perm_event and 'char
> extra_info_buf[FANOTIFY_MAX_RESPONSE_EXTRA_LEN]' for now for the data. If
> we ever grow larger extra info structures, we can always change this to
> dynamic allocation but that will be only internal fanotify change,
> userspace facing API can stay the same.

That fanotify/audit interface doesn't bother me as much since that is
internal and we can modify that as needed; the userspace/kernel
fanotify API and the audit record are the important things to focus
on.

Simply recording the "extra_info_type" integer and dumping the
"extra_info" as a hex encoded bitstring in the audit record is
probably the most future proof solution, but I'm not sure what Steve's
tooling would want from such a record.  It also seems to be in the
spirit of Steve's 3/3 patch.

-- 
paul moore
www.paul-moore.com

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-30 16:12 [PATCH 2/3] fanotify: define bit map fields to hold response decision context Steve Grubb
2020-10-01 10:12 ` Jan Kara
2020-10-01 10:24   ` Jan Kara
2020-10-01 20:33     ` Paul Moore

Linux-audit Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-audit/0 linux-audit/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-audit linux-audit/ https://lore.kernel.org/linux-audit \
		linux-audit@redhat.com
	public-inbox-index linux-audit

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/com.redhat.linux-audit


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git