Page MenuHomeFreeBSD
Differential D47104

syslogd: Open forwarding socket descriptors
AcceptedPublic
Actions

Authored by jfree on Oct 14 2024, 1:56 AM.
Tags
None
Referenced Files
F103578230: D47104.id146630.diff
Tue, Nov 26, 5:27 PM
Unknown Object (File)
Tue, Nov 26, 2:48 AM
Unknown Object (File)
Tue, Nov 26, 2:47 AM
Unknown Object (File)
Thu, Nov 21, 12:10 AM
Unknown Object (File)
Sat, Nov 16, 9:42 AM
Unknown Object (File)
Fri, Nov 8, 4:17 AM
Unknown Object (File)
Wed, Nov 6, 11:50 PM
Unknown Object (File)
Oct 21 2024, 4:05 AM
View All 14 Files
Subscribers
imp

Details

Reviewers
markj
Summary

Previously, when forwarding a message to a remote address, the target's
addrinfo was saved at config-parse-time. When message-deliver-time came,
the message's addrinfo was passed into sendmsg(2) and delivered by the
first available inet socket.

The use of sendmsg(2) is prohibited in Capsicum capability mode, so
sockets are now opened and connected to their remote peers at
config-parse-time when executing outside of the capability sandbox.

These sockets are then used with send(2), allowing forwarding to be
performed inside of the capability sandbox.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 60099
Build 56983: arc lint + arc unit

Event Timeline

jfree created this revision.Oct 14 2024, 1:56 AM
Herald added a subscriber: imp. · View Herald TranscriptOct 14 2024, 1:56 AM
jfree requested review of this revision.Oct 14 2024, 1:56 AM
Harbormaster completed remote builds in B59966: Diff 144797.Oct 14 2024, 1:56 AM
jfree added a parent revision: D41477: syslogd: Watch for dead pipe processes.Oct 14 2024, 1:57 AM
markj accepted this revision.Oct 14 2024, 12:45 PM

This looks good to me, thanks. A couple of minor nits.

usr.sbin/syslogd/syslogd.c
367–372

Hopefully f->f_addr_fds == NULL implies f->f_num_addr_fds == 0, in which case there's no need to check this condition explicitly.

3051

It'd be nicer to use calloc() here IMO.

This revision is now accepted and ready to land.Oct 14 2024, 12:45 PM
jfree updated this revision to Diff 145168.EditedOct 19 2024, 6:29 PM
jfree marked 2 inline comments as done.

Address Mark's comments: use calloc() instead of malloc() for
memory allocations. This is done to avoid potential integer overflow
in total allocation size.

This revision now requires review to proceed.Oct 19 2024, 6:29 PM
Harbormaster completed remote builds in B60099: Diff 145168.Oct 19 2024, 6:29 PM
jfree added inline comments.Oct 19 2024, 6:30 PM
usr.sbin/syslogd/syslogd.c
367–372

Yes. f->f_addr_fds == NULL does imply f->f_num_addr_fds == 0

markj accepted this revision.Oct 21 2024, 2:27 PM
markj added inline comments.
usr.sbin/syslogd/syslogd_cap_config.c
218

Same here, it'd be nice to use calloc().

This revision is now accepted and ready to land.Oct 21 2024, 2:27 PM
markj mentioned this in D41402: casper/cap_net: Expose pack/unpack functions.Oct 21 2024, 2:30 PM
jfree updated this revision to Diff 146623.Sun, Nov 17, 7:57 PM
jfree marked an inline comment as done.
  • Use calloc(3) instead of malloc(3) for array allocations.
  • Add missing error checking for a memory allocation
This revision now requires review to proceed.Sun, Nov 17, 7:57 PM
Harbormaster completed remote builds in B60646: Diff 146623.Sun, Nov 17, 7:57 PM
markj added inline comments.Sun, Nov 17, 10:06 PM
usr.sbin/syslogd/syslogd.c
1752

I'd suggest writing sockfd = f->f_addr_fds[0] here.

1769

The new getsockopt() and getpeername() calls are only needed to implement dprintf(), but that's a no-op unless debug printing is enabled. IMO it would be better to avoid this extra work unless Debug is true.

2674
3056
usr.sbin/syslogd/syslogd_cap_config.c
223

Not new with this diff, so can be fixed separately, but: can we use nvlist_take_descriptor_array() instead of duping? Same for the F_FILE case.

jfree updated this revision to Diff 146630.Mon, Nov 18, 2:59 AM
jfree marked 4 inline comments as done.
  • Minor style changes
  • Only call getsockopt() and getpeername() when Debug is non-zero
Harbormaster completed remote builds in B60653: Diff 146630.Mon, Nov 18, 2:59 AM
jfree added inline comments.Mon, Nov 18, 2:59 AM
usr.sbin/syslogd/syslogd_cap_config.c
223

Not new with this diff, so can be fixed separately, but: can we use nvlist_take_descriptor_array() instead of duping? Same for the F_FILE case.

We could. I addressed this in response to your other comment about it.

I use nvlist_get_descriptor_array() here because nvlist_to_filed() and filed_to_nvlist() do not consume their arguments. i.e. the nvlist passed into nvlist_to_filed() is marked const. Using nvlist_take_descriptor_array() would modify that nvlist.

markj added inline comments.Mon, Nov 18, 2:37 PM
usr.sbin/syslogd/syslogd.c
1802

Sorry I didn't catch this before - I believe you can still use sendmsg() (so as to avoid having to loop over the iovec), so long as msghdr.msg_name = NULL. That would be cleaner IMO.

jfree added inline comments.Wed, Nov 20, 6:04 PM
usr.sbin/syslogd/syslogd.c
1802

Sorry I didn't catch this before - I believe you can still use sendmsg() (so as to avoid having to loop over the iovec), so long as msghdr.msg_name = NULL. That would be cleaner IMO.

Ah! I could've sworn I checked syscalls.master and didn't see CAPENABLED on sendmsg() before doing this. I guess I didn't. I will definitely use sendmsg() then.

jfree updated this revision to Diff 146887.Sun, Nov 24, 5:48 PM
jfree marked an inline comment as done.

Use sendmsg(2) to send iovec list.

Harbormaster completed remote builds in B60747: Diff 146887.Sun, Nov 24, 5:48 PM
markj accepted this revision.Sun, Nov 24, 6:40 PM
This revision is now accepted and ready to land.Sun, Nov 24, 6:40 PM

Revision Contents
Changeset List

PathSize
usr.sbin/
syslogd/
3 lines
143 lines
33 lines

Diff 145168

View Options

usr.sbin/syslogd/syslogd.h

Loading...
View Options

usr.sbin/syslogd/syslogd.c

Loading...
View Options

usr.sbin/syslogd/syslogd_cap_config.c

Loading...