Bug 92831 - avc_init() function is deprecated
Summary: avc_init() function is deprecated
Status: RESOLVED MOVED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.10
Hardware: Other All
: medium minor
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review-
Keywords: patch
Depends on:
Blocks:
 
Reported: 2015-11-05 09:33 UTC by Laurent Bigonville
Modified: 2018-10-12 21:25 UTC (History)
3 users (show)

See Also:
i915 platform:
i915 features:


Attachments
WIP patch (8.63 KB, patch)
2018-03-04 12:13 UTC, Laurent Bigonville
Details | Splinter Review
Stop using avc_init() which is deprecated (9.85 KB, patch)
2018-03-05 13:00 UTC, Laurent Bigonville
Details | Splinter Review
Add _dbus_clear_loop and _dbus_clear_watch (1.47 KB, patch)
2018-03-05 22:49 UTC, Laurent Bigonville
Details | Splinter Review
Stop using avc_init() which is deprecated (10.76 KB, patch)
2018-03-05 22:49 UTC, Laurent Bigonville
Details | Splinter Review
Stop using avc_init() which is deprecated (10.76 KB, patch)
2018-03-05 22:52 UTC, Laurent Bigonville
Details | Splinter Review
Stop using avc_init() which is deprecated (11.37 KB, patch)
2018-03-12 16:49 UTC, Laurent Bigonville
Details | Splinter Review
Stop using avc_init() which is deprecated (12.14 KB, patch)
2018-05-30 16:13 UTC, Laurent Bigonville
Details | Splinter Review
Use SELINUX_CB_POLICYLOAD instead of AVC_CALLBACK_RESET callback (1.86 KB, patch)
2018-05-30 16:24 UTC, Laurent Bigonville
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Laurent Bigonville 2015-11-05 09:33:01 UTC
Hi,

From the avc_init(3) manpage, it seems that this function is deprecated and avc_open should be used instead:

avc_init() is deprecated; please use avc_open(3) in conjunction with selinux_set_callback(3) in all new code.
Comment 1 Simon McVittie 2015-11-06 08:52:34 UTC
Please send a tested patch? None of the D-Bus maintainers (except possibly Colin) use or know about SELinux.
Comment 2 Colin Walters 2015-11-08 15:59:27 UTC
See also https://bugzilla.redhat.com/show_bug.cgi?id=1278602
Comment 3 Laurent Bigonville 2015-11-08 21:19:07 UTC
I've also opened this bug in fdo bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92832
Comment 4 Simon McVittie 2015-12-02 11:37:37 UTC
<https://bugzilla.redhat.com/show_bug.cgi?id=1278602#c19>
I wrote:
> If we call avc_netlink_acquire_fd, watch that fd for POLLIN events, call
> avc_netlink_check_nb() whenever we see POLLIN on that fd, and call
> avc_netlink_release_fd() before avc_destroy(), is that valid? That seems
> to be what Xorg does.

<https://bugzilla.redhat.com/show_bug.cgi?id=1278602#c20>
Stephen Smalley replied:
> Yes, that should work.  You should then be able to get rid of the thread
> and lock callbacks for avc_init (the latter assuming that dbus is
> otherwise single-threaded).

This is my preferred solution for git master (1.11.x). Anyone want to implement and test this? It should use a DBusWatch for DBUS_WATCH_READABLE, a bit like bus/dir-watch-inotify.c.

I might put together a patch myself at some point if I have time, but I'll still be relying on SELinux users to test it. I'm not going to merge this untested.

I don't think it's a good idea to change this in 1.10.x.
Comment 5 Simon McVittie 2016-11-29 15:06:54 UTC
David, since you've chimed in on the other audit bug Bug #92832: would you be able to get someone who knows this stuff to write a patch as outlined in Comment #4, or review one if I wrote it?
Comment 6 Laurent Bigonville 2018-03-04 12:13:32 UTC
Created attachment 137773 [details] [review]
WIP patch

Hi,

Here a WIP patch for this issue.

Do you think I'm going in the right direction?

The tests are not compiling anymore due to the signature change of bus_selinux_full_init() (I'm passing the context), an idea how to fix that?
Comment 7 Simon McVittie 2018-03-05 12:07:15 UTC
(In reply to Laurent Bigonville from comment #6)
> Here a WIP patch for this issue.
> 
> Do you think I'm going in the right direction?

Yes, looks like you are. Thanks for looking into this - I know very little about SELinux and I've never used it myself. Perhaps Red Hat/Fedora people could help with test and review since it's their default LSM?

> The tests are not compiling anymore due to the signature change of
> bus_selinux_full_init() (I'm passing the context), an idea how to fix that?

Moving the initialization from test_pre_hook() to be close to bus_context_new() would probably work? Take a look at the real bus/main.c and mimic the initialization order in bus_context_new_test(), perhaps.

(Note that the initialization order has changed several times as we found additional pitfalls in how LSMs, auditing and dropping privileges interrelate, most recently in Bug #105165.)
Comment 8 Simon McVittie 2018-03-05 12:37:06 UTC
Comment on attachment 137773 [details] [review]
WIP patch

Review of attachment 137773 [details] [review]:
-----------------------------------------------------------------

I don't know much about SELinux, so I'm only reviewing from the dbus perspective.

::: bus/selinux.c
@@ +69,5 @@
> +static int avc_netlink_fd = -1;
> +
> +/* Watch to listen for SELinux status changes via netlink. */
> +static DBusWatch *avc_netlink_watch_obj = NULL;
> +static DBusLoop *avc_netlink_loop_obj = NULL;

bus/dir-watch-inotify should be quite a good basis for "a non-essential module hooks into the main loop" if you're not already stealing code from there.

@@ +91,5 @@
>   */
>  #ifdef HAVE_SELINUX
>  
> +static int
> +log_callback (int type, const char *fmt, ...)

Should this be doing something with @type, perhaps using it in a switch statement to vary the vsyslog() priority or the audit_log_user_avc_message() type, now that we know it?

@@ +175,2 @@
>  {
> +  if (avc_netlink_check_nb() < 0)

Coding style: space before opening parenthesis in new code, please.

@@ +177,2 @@
>      {
> +      _dbus_warn("Failed to check the netlink socket for pending messages and process them");

How bad is this, on a scale from "benign" to "can't happen"?

It should maybe use bus_context_log() now that we don't need to worry about multi-threading.

It should probably substitute _dbus_strerror (errno) into the error message, assuming avc_netlink_check_nb() sets errno on failure.

@@ +260,4 @@
>   * logging callbacks.
>   */
>  dbus_bool_t
> +bus_selinux_full_init (BusContext *context)

I can't help wondering whether this should raise a DBusError, like bus_apparmor_full_init() does. The fact that it warns and just returns a boolean is not really in line with libdbus coding style ("just return a boolean" is meant to be very well-correlated with "the only possible error is OOM") - but I didn't convert it to DBusError yet because I have no way to test it.

@@ +287,2 @@
>      {
>        _dbus_warn ("Failed to start Access Vector Cache (AVC).");

This should probably log the stringified errno, assuming avc_open() sets it. (Or include it in the DBusError instead if you agree that raising a DBusError is appropriate.)

@@ +295,5 @@
>  
> +  avc_netlink_fd = avc_netlink_acquire_fd();
> +  if (avc_netlink_fd <= 0)
> +    {
> +       _dbus_warn ("Cannot acquire avc netlink fd");

Shouldn't you avc_destroy() here?

Print errno, as above.

@@ +305,5 @@
> +  avc_netlink_loop_obj = bus_context_get_loop (context);
> +  _dbus_loop_ref (avc_netlink_loop_obj);
> +
> +  avc_netlink_watch_obj = _dbus_watch_new (avc_netlink_fd, DBUS_WATCH_READABLE, TRUE,
> +                               handle_avc_netlink_watch, NULL, NULL);

Coding style: please indent with spaces (not hard tabs), and either line up continuations with the opening parenthesis, or indent them at beginning of line + 4 spaces.

@@ +311,5 @@
> +  if (!_dbus_loop_add_watch (avc_netlink_loop_obj, avc_netlink_watch_obj))
> +    {
> +      _dbus_warn ("Unable to add reload watch to main loop");
> +      _dbus_watch_unref (avc_netlink_watch_obj);
> +      avc_netlink_watch_obj = NULL;

Shouldn't you close the fd and call avc_destroy() too, so that all actions taken by this function are undone before exiting?

@@ +323,5 @@
>                    _dbus_strerror (errno));
> +      _dbus_watch_unref (avc_netlink_watch_obj);
> +      avc_netlink_watch_obj = NULL;
> +      avc_netlink_release_fd ();
> +      avc_netlink_fd = -1;

There's a lot of duplicate cleanup code here, which in my experience is often slightly wrong, and my review of the cleanup code above here indicates that this is probably no exception.

Perhaps the "goto error" or "goto out" pattern would be easier to get right? (Those are dbus' conventional labels for the equivalents of Python "except" and "finally".)

I'm a fan of this pattern for fds, pointers, etc.:

  Resource foo = INVALID_VALUE;
  Object *bar = NULL;

  if (!create_foo (&foo))
    goto error;

  ...

  bar = do_something_else ();

  if (bar == NULL)
    goto error;

  ...

  return TRUE;

error:
  /* undo resource allocations in reverse order */

  if (bar != NULL)
    object_free (bar);

  if (foo != INVALID_VALUE)
    undo (foo);

  return FALSE;

@@ +935,5 @@
> +    {
> +      _dbus_loop_remove_watch (avc_netlink_loop_obj, avc_netlink_watch_obj);
> +      _dbus_watch_invalidate (avc_netlink_watch_obj);
> +      _dbus_watch_unref (avc_netlink_watch_obj);
> +      _dbus_loop_unref (avc_netlink_loop_obj);

I'd prefer to be defensive against the loop being non-NULL even though the watch is NULL.

If you add a _dbus_clear_watch() and _dbus_clear_loop() similar to dbus_clear_connection() (as a separate commit before this one), you could use those to simplify, a lot like g_clear_object().

@@ +947,5 @@
>  
>        bus_avc_print_stats ();
>  
> +      avc_netlink_release_fd ();
> +      avc_netlink_fd = -1;

Maybe guard these fd operations (and avc_destroy(), and possibly even bus_avc_print_stats()) with (avc_netlink_fd != -1) instead of (bus_sid != SECSID_WILD)?
Comment 9 Laurent Bigonville 2018-03-05 13:00:01 UTC
Created attachment 137794 [details] [review]
Stop using avc_init() which is deprecated

Stop using avc_init() and use avc_open() instead. With this commit
dbus-daemon will stop using a thread to monitor the avc netlink and will
poll it instead.
Comment 10 Laurent Bigonville 2018-03-05 13:02:12 UTC
The last patch is compiling at least

I'll address the other remarks later today
Comment 11 Laurent Bigonville 2018-03-05 22:25:47 UTC
> ::: bus/selinux.c
> @@ +69,5 @@
> > +static int avc_netlink_fd = -1;
> > +
> > +/* Watch to listen for SELinux status changes via netlink. */
> > +static DBusWatch *avc_netlink_watch_obj = NULL;
> > +static DBusLoop *avc_netlink_loop_obj = NULL;
> 
> bus/dir-watch-inotify should be quite a good basis for "a non-essential
> module hooks into the main loop" if you're not already stealing code from
> there.

I already looked at that and I copied the code I believe.

> 
> @@ +91,5 @@
> >   */
> >  #ifdef HAVE_SELINUX
> >  
> > +static int
> > +log_callback (int type, const char *fmt, ...)
> 
> Should this be doing something with @type, perhaps using it in a switch
> statement to vary the vsyslog() priority or the audit_log_user_avc_message()
> type, now that we know it?

I'm not exactly sure how to map that to syslog priority, that can be added later I guess?

> 
> @@ +177,2 @@
> >      {
> > +      _dbus_warn("Failed to check the netlink socket for pending messages and process them");
> 
> How bad is this, on a scale from "benign" to "can't happen"?

Well, if the message is not processed, that means that some libselinux internal caches might not be cleaned, so that's not good as some decisions from avc_has_perm() might be wrong.

> 
> It should maybe use bus_context_log() now that we don't need to worry about
> multi-threading.

Should bus_context_log() be used everywhere?

> 
> It should probably substitute _dbus_strerror (errno) into the error message,
> assuming avc_netlink_check_nb() sets errno on failure.
> 
> @@ +260,4 @@
> >   * logging callbacks.
> >   */
> >  dbus_bool_t
> > +bus_selinux_full_init (BusContext *context)
> 
> I can't help wondering whether this should raise a DBusError, like
> bus_apparmor_full_init() does. The fact that it warns and just returns a
> boolean is not really in line with libdbus coding style ("just return a
> boolean" is meant to be very well-correlated with "the only possible error
> is OOM") - but I didn't convert it to DBusError yet because I have no way to
> test it.

Maybe that can be done later? Not too sure how proceed here.

Setting up a debian with SELinux is not too difficult :) (Fedora/Centos have SELinux running by default)

> 
> @@ +287,2 @@
> >      {
> >        _dbus_warn ("Failed to start Access Vector Cache (AVC).");
> 
> This should probably log the stringified errno, assuming avc_open() sets it.
> (Or include it in the DBusError instead if you agree that raising a
> DBusError is appropriate.)

I've added it for now.

> 
> @@ +295,5 @@
> >  
> > +  avc_netlink_fd = avc_netlink_acquire_fd();
> > +  if (avc_netlink_fd <= 0)
> > +    {
> > +       _dbus_warn ("Cannot acquire avc netlink fd");
> 
> Shouldn't you avc_destroy() here?
> 
> Print errno, as above.

I actually think that avc_netlink_acquire_fd() cannot fail if avc_open() succeed.

> 
> @@ +305,5 @@
> > +  avc_netlink_loop_obj = bus_context_get_loop (context);
> > +  _dbus_loop_ref (avc_netlink_loop_obj);
> > +
> > +  avc_netlink_watch_obj = _dbus_watch_new (avc_netlink_fd, DBUS_WATCH_READABLE, TRUE,
> > +                               handle_avc_netlink_watch, NULL, NULL);
> 
> Coding style: please indent with spaces (not hard tabs), and either line up
> continuations with the opening parenthesis, or indent them at beginning of
> line + 4 spaces.

Done

> 
> @@ +311,5 @@
> > +  if (!_dbus_loop_add_watch (avc_netlink_loop_obj, avc_netlink_watch_obj))
> > +    {
> > +      _dbus_warn ("Unable to add reload watch to main loop");
> > +      _dbus_watch_unref (avc_netlink_watch_obj);
> > +      avc_netlink_watch_obj = NULL;
> 
> Shouldn't you close the fd and call avc_destroy() too, so that all actions
> taken by this function are undone before exiting?
> 
> @@ +323,5 @@
> >                    _dbus_strerror (errno));
> > +      _dbus_watch_unref (avc_netlink_watch_obj);
> > +      avc_netlink_watch_obj = NULL;
> > +      avc_netlink_release_fd ();
> > +      avc_netlink_fd = -1;
> 
> There's a lot of duplicate cleanup code here, which in my experience is
> often slightly wrong, and my review of the cleanup code above here indicates
> that this is probably no exception.
> 
> Perhaps the "goto error" or "goto out" pattern would be easier to get right?
> (Those are dbus' conventional labels for the equivalents of Python "except"
> and "finally".)
> 
> I'm a fan of this pattern for fds, pointers, etc.:
> 
>   Resource foo = INVALID_VALUE;
>   Object *bar = NULL;
> 
>   if (!create_foo (&foo))
>     goto error;
> 
>   ...
> 
>   bar = do_something_else ();
> 
>   if (bar == NULL)
>     goto error;
> 
>   ...
> 
>   return TRUE;
> 
> error:
>   /* undo resource allocations in reverse order */
> 
>   if (bar != NULL)
>     object_free (bar);
> 
>   if (foo != INVALID_VALUE)
>     undo (foo);
> 
>   return FALSE;
> 

I've done something similar

> @@ +935,5 @@
> > +    {
> > +      _dbus_loop_remove_watch (avc_netlink_loop_obj, avc_netlink_watch_obj);
> > +      _dbus_watch_invalidate (avc_netlink_watch_obj);
> > +      _dbus_watch_unref (avc_netlink_watch_obj);
> > +      _dbus_loop_unref (avc_netlink_loop_obj);
> 
> I'd prefer to be defensive against the loop being non-NULL even though the
> watch is NULL.
> 
> If you add a _dbus_clear_watch() and _dbus_clear_loop() similar to
> dbus_clear_connection() (as a separate commit before this one), you could
> use those to simplify, a lot like g_clear_object().
> 

Added

> @@ +947,5 @@
> >  
> >        bus_avc_print_stats ();
> >  
> > +      avc_netlink_release_fd ();
> > +      avc_netlink_fd = -1;
> 
> Maybe guard these fd operations (and avc_destroy(), and possibly even
> bus_avc_print_stats()) with (avc_netlink_fd != -1) instead of (bus_sid !=
> SECSID_WILD)?
Comment 12 Laurent Bigonville 2018-03-05 22:49:26 UTC
Created attachment 137801 [details] [review]
Add _dbus_clear_loop and _dbus_clear_watch
Comment 13 Laurent Bigonville 2018-03-05 22:49:30 UTC
Created attachment 137802 [details] [review]
Stop using avc_init() which is deprecated

Stop using avc_init() and use avc_open() instead. With this commit
dbus-daemon will stop using a thread to monitor the avc netlink and will
poll it instead.
Comment 14 Laurent Bigonville 2018-03-05 22:52:08 UTC
Created attachment 137803 [details] [review]
Stop using avc_init() which is deprecated

Stop using avc_init() and use avc_open() instead. With this commit
dbus-daemon will stop using a thread to monitor the avc netlink and will
poll it instead.
Comment 15 Laurent Bigonville 2018-03-06 09:45:38 UTC
> 
> @@ +177,2 @@
> >      {
> > +      _dbus_warn("Failed to check the netlink socket for pending messages and process them");
> 
> How bad is this, on a scale from "benign" to "can't happen"?

> Well, if the message is not processed, that means that some libselinux internal caches might not be cleaned, so that's not good as some decisions from avc_has_perm() might be wrong.

Apparently when avc_netlink_check_nb() is called internally in libselinux, the return code is ignored, so I'm not sure if it's that critical.

I'm also wondering if avc_add_callback() couldn't be replaced by selinux_set_callback(), with the later, the callback is called a tiny bit later in the execution
Comment 16 Simon McVittie 2018-03-06 12:37:19 UTC
Comment on attachment 137801 [details] [review]
Add _dbus_clear_loop and _dbus_clear_watch

Review of attachment 137801 [details] [review]:
-----------------------------------------------------------------

I'll apply this to master with the obvious fix to the variable names.

::: dbus/dbus-mainloop.h
@@ +60,5 @@
>  int  _dbus_get_oom_wait    (void);
>  void _dbus_wait_for_memory (void);
>  
> +static inline void
> +_dbus_clear_loop (DBusLoop **pointer_to_connection)

pointer_to_loop :-)

::: dbus/dbus-watch.h
@@ +99,5 @@
>  DBUS_PRIVATE_EXPORT
>  DBusPollable   _dbus_watch_get_pollable       (DBusWatch               *watch);
>  
> +static inline void
> +_dbus_clear_watch (DBusWatch **pointer_to_connection)

Similar
Comment 17 Simon McVittie 2018-03-06 12:54:50 UTC
Comment on attachment 137803 [details] [review]
Stop using avc_init() which is deprecated

Review of attachment 137803 [details] [review]:
-----------------------------------------------------------------

::: bus/selinux.c
@@ +69,5 @@
> +static int avc_netlink_fd = -1;
> +
> +/* Watch to listen for SELinux status changes via netlink. */
> +static DBusWatch *avc_netlink_watch_obj = NULL;
> +static DBusLoop *avc_netlink_loop_obj = NULL;

Is the _obj suffix significant? It can probably be dropped, since we know that watches and loops are objects.

@@ +260,4 @@
>   * logging callbacks.
>   */
>  dbus_bool_t
> +bus_selinux_full_init (BusContext *context)

I still think this should raise a DBusError, like its AppArmor counterpart, instead of using _dbus_warn(). The pattern is a lot like GError:

dbus_bool_t
something (..., DBusError *error)
{
  if (!this_returns_false_on_OOM ())
    {
      BUS_SET_OOM (error);
      goto error;
    }

  if (!this_has_an_error_parameter (..., error))
    {
      goto error;
    }

  if (this_sets_errno (...) < 0)
    {
      dbus_set_error (error, DBUS_ERROR_FAILED,
                      "Cannot reticulate splines: %s",
                      _dbus_strerror (errno));
      goto error;
    }

  return TRUE;

error:
  ... clean up ...
  _DBUS_ASSERT_ERROR_IS_SET (error);
  return FALSE;
}

@@ +305,5 @@
> +  avc_netlink_loop_obj = bus_context_get_loop (context);
> +  _dbus_loop_ref (avc_netlink_loop_obj);
> +
> +  avc_netlink_watch_obj = _dbus_watch_new (avc_netlink_fd, DBUS_WATCH_READABLE, TRUE,
> +                                           handle_avc_netlink_watch, NULL, NULL);

if (avc_netlink_watch_obj == NULL)
  {
    ... handle "not enough memory" error ...
  }

@@ +347,5 @@
> +
> +  return TRUE;
> +
> +error:
> +  if (avc_netlink_watch_obj) {

Coding style: GNUish

if (foo)
  {
    bar;
  }

instead of

if (foo) {
  bar;
}

@@ +352,5 @@
> +    _dbus_loop_remove_watch (avc_netlink_loop_obj, avc_netlink_watch_obj);
> +    _dbus_watch_invalidate (avc_netlink_watch_obj);
> +    _dbus_clear_watch (&avc_netlink_watch_obj);
> +  }
> +  if (avc_netlink_loop_obj) {

You can omit the condition and just call _dbus_clear_loop() unconditionally here - it's like g_clear_object().

@@ +359,5 @@
> +  if (avc_netlink_fd >= 0) {
> +    avc_netlink_release_fd ();
> +    avc_netlink_fd = -1;
> +  }
> +  avc_destroy ();

Is it valid to call avc_destroy() if it was never successfully opened?

@@ +942,4 @@
>  
>    _dbus_verbose ("AVC shutdown\n");
>  
> +  avc_netlink_loop_obj = NULL;

This is never unreffed as a result of setting it to NULL here. I think you should probably just delete this?

@@ +944,5 @@
>  
> +  avc_netlink_loop_obj = NULL;
> +  if (avc_netlink_watch_obj)
> +    {
> +      _dbus_loop_remove_watch (avc_netlink_loop_obj, avc_netlink_watch_obj);

avc_netlink_loop_obj is NULL here.

@@ +948,5 @@
> +      _dbus_loop_remove_watch (avc_netlink_loop_obj, avc_netlink_watch_obj);
> +      _dbus_watch_invalidate (avc_netlink_watch_obj);
> +      _dbus_clear_watch (&avc_netlink_watch_obj);
> +    }
> +  if (avc_netlink_loop_obj)

You can omit the condition and call _dbus_clear_loop (&avc_netlink_loop_obj) unconditionally - it's like g_clear_object().
Comment 18 Simon McVittie 2018-03-06 13:02:20 UTC
(In reply to Laurent Bigonville from comment #11)
> > It should maybe use bus_context_log() now that we don't need to worry about
> > multi-threading.
> 
> Should bus_context_log() be used everywhere?

If a function has success and failure code paths, and failure causes the function to abort processing ("raise an exception"), then that should usually be represented by a DBusError; a higher level of stack can "catch the exception" and log it.

Other than that, _dbus_warn() is usually used for "can't happen" situations that indicate that generic/library code has encountered undefined behaviour or a broken D-Bus installation, whereas bus_context_log() is usually used for diagnostics in the dbus-daemon.

DBUS_SYSTEM_LOG_WARNING often indicates that the dbus-daemon is working OK (as much as it can under the circumstances) but the rest of the system might not be; DBUS_SYSTEM_LOG_ERROR is currently only used when there's been a fatal error in SELinux initialization.
Comment 19 Laurent Bigonville 2018-03-12 16:47:32 UTC
(In reply to Simon McVittie from comment #17)
> Comment on attachment 137803 [details] [review] [review]
> Stop using avc_init() which is deprecated
> 
> Review of attachment 137803 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: bus/selinux.c
> @@ +69,5 @@
> > +static int avc_netlink_fd = -1;
> > +
> > +/* Watch to listen for SELinux status changes via netlink. */
> > +static DBusWatch *avc_netlink_watch_obj = NULL;
> > +static DBusLoop *avc_netlink_loop_obj = NULL;
> 
> Is the _obj suffix significant? It can probably be dropped, since we know
> that watches and loops are objects.

The problem is that there is a function called avc_netlink_loop() already, so I wanted to have a different name here :/

[...]
> @@ +359,5 @@
> > +  if (avc_netlink_fd >= 0) {
> > +    avc_netlink_release_fd ();
> > +    avc_netlink_fd = -1;
> > +  }
> > +  avc_destroy ();
> 
> Is it valid to call avc_destroy() if it was never successfully opened?

We exit the function immediately if avc_open() fails, so we shouldn't be in that case, right?
Comment 20 Laurent Bigonville 2018-03-12 16:49:55 UTC
Created attachment 138031 [details] [review]
Stop using avc_init() which is deprecated

Stop using avc_init() and use avc_open() instead. With this commit
dbus-daemon will stop using a thread to monitor the avc netlink and will
poll it instead.
Comment 21 Laurent Bigonville 2018-04-17 15:47:15 UTC
Hi,

The different patches for this bug and bug #105330 have been reviewed by Stephen Smalley, see https://marc.info/?t=152163375000002&r=1&w=2

I can also provide and other patch to use selinux_set_callback() instead of avc_add_callback()
Comment 22 Simon McVittie 2018-04-17 16:21:10 UTC
Comment on attachment 138031 [details] [review]
Stop using avc_init() which is deprecated

Review of attachment 138031 [details] [review]:
-----------------------------------------------------------------

::: bus/bus.c
@@ +995,4 @@
>     */
>    bus_audit_init (context);
>  
> +  if (!bus_selinux_full_init (context, error))

Now that this raises a DBusError, you should let bus_context_new() propagate the error to its caller (a lot like the AppArmor code path immediately below this one) instead of "catching the exception", ignoring its contents and exit()ing yourself. It will end up in main() where dbus-daemon logs a warning containing error->message and exits 1.

::: bus/selinux.c
@@ +297,5 @@
>  
> +  avc_netlink_fd = avc_netlink_acquire_fd();
> +  if (avc_netlink_fd < 0)
> +    {
> +       _dbus_warn ("Cannot acquire avc netlink fd");

Set the error here instead of printing a warning, otherwise the _DBUS_ASSERT_ERROR_IS_SET (error) after the "error" label will fail.

If avc_netlink_acquire_fd() sets errno, then

dbus_set_error (error, DBUS_ERROR_FAILED,
                "Cannot acquire AVC netlink fd: %s",
                _dbus_strerror (errno));
goto error;

would be a suitable replacement. If it doesn't set errno, use whatever error-reporting mechanism it does have, or in the worst case leave it vague.

@@ +316,5 @@
> +    }
> +
> +  if (!_dbus_loop_add_watch (avc_netlink_loop_obj, avc_netlink_watch_obj))
> +    {
> +      _dbus_warn ("Unable to add reload watch to main loop");

BUS_SET_OOM (error) instead of printing a warning (_dbus_loop_add_watch() is documented to only fail on OOM).

If you set the error on one failure code path you have to set it on all failure code paths.

@@ +338,5 @@
>  
>    if (getcon (&bus_context) < 0)
>      {
> +      dbus_set_error (error, DBUS_ERROR_FAILED,
> +                      "Error getting context of bus: %s\n",

You don't need the \n here: DBusErrors conventionally don't end with a newline (whoever prints or logs them is responsible for adding a newline if necessary, and functions like _dbus_warn() automatically add a newline).

@@ +346,5 @@
>        
>    if (avc_context_to_sid (bus_context, &bus_sid) < 0)
>      {
> +      dbus_set_error (error, DBUS_ERROR_FAILED,
> +                      "Error getting SID from bus context: %s\n",

Similarly, no newline here

@@ +359,5 @@
> +
> +error:
> +  if (avc_netlink_watch_obj)
> +    {
> +      _dbus_loop_remove_watch (avc_netlink_loop_obj, avc_netlink_watch_obj);

This will _dbus_warn() "could not find watch %p to remove" if the loop was allocated but not successfully added (due to OOM), but we're about to crash out anyway so perhaps that isn't a real problem.

If you want to fix that (just for completeness really), you could invalidate and clear avc_netlink_watch_obj yourself before the "goto error" if adding it to the main loop fails; or you could have "dbus_bool_t added_watch = FALSE", set it to TRUE after successfully adding it, and only remove it if it was successfully added.
Comment 23 Simon McVittie 2018-04-17 16:24:10 UTC
(In reply to Laurent Bigonville from comment #19)
> (In reply to Simon McVittie from comment #17)
> > Is the _obj suffix significant? It can probably be dropped, since we know
> > that watches and loops are objects.
> 
> The problem is that there is a function called avc_netlink_loop() already,
> so I wanted to have a different name here :/

OK. The name is a bit unwieldy, but it's fine.

> > Is it valid to call avc_destroy() if it was never successfully opened?
> 
> We exit the function immediately if avc_open() fails, so we shouldn't be in
> that case, right?

OK, not a problem then. I'd missed the fact that a failed avc_open() didn't "goto error" like the rest of the failure code paths.
Comment 24 Simon McVittie 2018-04-17 16:28:48 UTC
(In reply to Laurent Bigonville from comment #21)
> I can also provide and other patch to use selinux_set_callback() instead of
> avc_add_callback()

If people who know about SELinux/AVC say this is a good thing to do as a follow-up patch, then I'm not going to contradict them. The more justification you can provide in the commit message, the better (as a non-SELinux-user I'm relying on SELinux experts to provide context for this sort of thing).

When Stephen Smalley talks about "dbusd" does he mean dbus-daemon, or does he mean some reimplementation of dbus-daemon?
Comment 25 Laurent Bigonville 2018-05-30 16:13:30 UTC
Created attachment 139865 [details] [review]
Stop using avc_init() which is deprecated

Stop using avc_init() and use avc_open() instead. With this commit
dbus-daemon will stop using a thread to monitor the avc netlink and will
poll it instead.
Comment 26 Laurent Bigonville 2018-05-30 16:24:25 UTC
Created attachment 139866 [details] [review]
Use SELINUX_CB_POLICYLOAD instead of AVC_CALLBACK_RESET callback

Use SELINUX_CB_POLICYLOAD instead of AVC_CALLBACK_RESET callback as this
only seems necessary on policy reload and not if the enforcing mode is
changing.

See discussion at https://marc.info/?l=selinux&m=152173501930182&w=2
Comment 27 Laurent Bigonville 2018-05-30 16:32:23 UTC
(In reply to Simon McVittie from comment #24)
> When Stephen Smalley talks about "dbusd" does he mean dbus-daemon, or does
> he mean some reimplementation of dbus-daemon?

I think he means dbus-daemon here
Comment 28 GitLab Migration User 2018-10-12 21:25:07 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/134.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.