ps: accept numerical user IDs
ps(1) -U expects names, top(1) and pgrep(1) -U take numbers as well.
With the new tree view, I start using 'ps -fU55' more often instead of
'top -U55' to watch ports builds, but keep doing
$ ps -fU55
ps: p55: no such user
$ ps -fU_pbuild
...
The stray "p" in current error messages comes kludge_oldps_options()
treats numbers as PIDs, but that makes no sense for a mandatory -U
argument, imho, so exlucde -U here to make '-U55' work like '-U 55'.
Make ps take both formats and use top's argument name/error message
for consistency.
$ ./ojb/ps -fUnope
ps: nope: unknown user
$ ps -fU55
...
Did I miss anything wrt. ps' old vs. new option parsing logic?
Feedback? Objection? OK?
Index: ps.1
===
RCS file: /cvs/src/bin/ps/ps.1,v
retrieving revision 1.129
diff -u -p -r1.129 ps.1
--- ps.113 Oct 2022 21:37:05 - 1.129
+++ ps.116 Apr 2023 01:10:28 -
@@ -47,7 +47,7 @@
.Op Fl o Ar fmt
.Op Fl p Ar pid
.Op Fl t Ar tty
-.Op Fl U Ar username
+.Op Fl U Ar user
.Op Fl W Ar swap
.Sh DESCRIPTION
The
@@ -150,9 +150,10 @@ with the standard input.
.It Fl t Ar tty
Display information about processes attached to the specified terminal
device.
-.It Fl U Ar username
+.It Fl U Ar user
Display the processes belonging to the specified
-.Ar username .
+username or UID
+.Ar user .
.It Fl u
Display information associated with the following keywords:
user, pid, %cpu, %mem, vsz, rss, tt, state, start, time, and command.
Index: ps.c
===
RCS file: /cvs/src/bin/ps/ps.c,v
retrieving revision 1.79
diff -u -p -r1.79 ps.c
--- ps.c1 Sep 2022 21:15:54 - 1.79
+++ ps.c16 Apr 2023 01:10:29 -
@@ -226,11 +226,24 @@ main(int argc, char *argv[])
ttydev = sb.st_rdev;
break;
}
- case 'U':
- if (uid_from_user(optarg, ) == -1)
- errx(1, "%s: no such user", optarg);
+ case 'U': {
+ int found = 0;
+
+ if (uid_from_user(optarg, ) == 0)
+ found = 1;
+ else {
+ const char *errstr;
+
+ uid = strtonum(optarg, 0, UID_MAX, );
+ if (errstr == NULL &&
+ user_from_uid(uid, 1) != NULL)
+ found = 1;
+ }
+ if (!found)
+ errx(1, "%s: unknown user", optarg);
Uflag = xflg = 1;
break;
+ }
case 'u':
parsefmt(ufmt);
sortby = SORTCPU;
@@ -480,11 +493,12 @@ kludge_oldps_options(char *s)
memmove(ns, s, (size_t)(cp - s)); /* copy up to trailing number */
ns += cp - s;
/*
-* if there's a trailing number, and not a preceding 'p' (pid) or
-* 't' (tty) flag, then assume it's a pid and insert a 'p' flag.
+* if there's a trailing number, and not a preceding 'p' (pid),
+* 't' (tty) or 'U' (user) flag,
+* then assume it's a pid and insert a 'p' flag.
*/
if (isdigit((unsigned char)*cp) &&
- (cp == s || (cp[-1] != 't' && cp[-1] != 'p' &&
+ (cp == s || (cp[-1] != 't' && cp[-1] != 'p' && cp[-1] != 'U' &&
(cp - 1 == s || cp[-2] != 't'
*ns++ = 'p';
/* and append the number */
@@ -611,7 +625,7 @@ usage(void)
{
fprintf(stderr, "usage: %s [-AacefHhjkLlmrSTuvwx] [-M core] [-N system]"
" [-O fmt] [-o fmt] [-p pid]\n", __progname);
- fprintf(stderr, "%-*s[-t tty] [-U username] [-W swap]\n",
+ fprintf(stderr, "%-*s[-t tty] [-U user] [-W swap]\n",
(int)strlen(__progname) + 8, "");
exit(1);
}
Re: pass M_CANFAIL to malloc() which use M_WAITOK but are tested for failure
On Fri, Apr 14, 2023 at 02:01:29PM +0200, Alexander Bluhm wrote: > > On Thu, Apr 13, 2023 at 10:43:30AM +0800, Kevin Lo wrote: > > M_CANFAIL > > In the M_WAITOK case, if not enough memory is available, > > return NULL instead of calling panic(9). If mallocarray() > > Did you see such a panic? If yes it would be better to understand > and fix the root cause. No, I didn't see that. > > detects an overflow or malloc() detects an excessive > > allocation, return NULL instead of calling panic(9). > > > > > > I'd like to keep the mallocarray call unchanged so I add M_CANFAIL > > to the flags. > > I think you are trying to change the kernel in the wrong direction. > It should not fail, but handle the requests. Panic if there is a > bug. > > Why do you think M_CANFAIL is a good thing at this place? Because M_WAITOK will not return NULl, I think adding M_CANFAIL will keep the mallocarray call unchanged. > bluhm >
Re: in_ioctl*: hoist identical privilege checks
On Fri, Apr 14, 2023 at 11:33:18PM +, Klemens Nanni wrote:
> All cases do the same check up first, so merge it before the switch.
>
> It could be hoisted further in both in_ioctl() and in_ioctl_change_ifaddr(),
> but that meant a change in errno return semantic, so leave it for now.
in6.c already has the privilege check as early as possible, so here's a diff
that simplifies in.c to match in6.c.
I can't think of a scenario where returning EPERM (this diff) instead of
whatever errno the currently earlier sanity checks yield would break.
It is an unprivileged ioctl call in the first place, so EPERM as soon as
possible makes sense to me.
This nicely drops 23 lines and a needless argument in two functions,
matching in6_ioctl*().
Feedback? Objection? OK?
Index: netinet/in.c
===
RCS file: /cvs/src/sys/netinet/in.c,v
retrieving revision 1.180
diff -u -p -r1.180 in.c
--- netinet/in.c15 Apr 2023 13:24:47 - 1.180
+++ netinet/in.c15 Apr 2023 13:45:56 -
@@ -84,8 +84,8 @@
void in_socktrim(struct sockaddr_in *);
-int in_ioctl_set_ifaddr(u_long, caddr_t, struct ifnet *, int);
-int in_ioctl_change_ifaddr(u_long, caddr_t, struct ifnet *, int);
+int in_ioctl_set_ifaddr(u_long, caddr_t, struct ifnet *);
+int in_ioctl_change_ifaddr(u_long, caddr_t, struct ifnet *);
int in_ioctl_get(u_long, caddr_t, struct ifnet *);
void in_purgeaddr(struct ifaddr *);
int in_addhost(struct in_ifaddr *, struct sockaddr_in *);
@@ -199,10 +199,9 @@ in_sa2sin(struct sockaddr *sa, struct so
int
in_control(struct socket *so, u_long cmd, caddr_t data, struct ifnet *ifp)
{
- int privileged;
+ int privileged = 0;
int error;
- privileged = 0;
if ((so->so_state & SS_PRIV) != 0)
privileged++;
@@ -242,10 +241,14 @@ in_ioctl(u_long cmd, caddr_t data, struc
case SIOCGIFBRDADDR:
return in_ioctl_get(cmd, data, ifp);
case SIOCSIFADDR:
- return in_ioctl_set_ifaddr(cmd, data, ifp, privileged);
+ if (!privileged)
+ return (EPERM);
+ return in_ioctl_set_ifaddr(cmd, data, ifp);
case SIOCAIFADDR:
case SIOCDIFADDR:
- return in_ioctl_change_ifaddr(cmd, data, ifp, privileged);
+ if (!privileged)
+ return (EPERM);
+ return in_ioctl_change_ifaddr(cmd, data, ifp);
case SIOCSIFNETMASK:
case SIOCSIFDSTADDR:
case SIOCSIFBRDADDR:
@@ -282,13 +285,13 @@ in_ioctl(u_long cmd, caddr_t data, struc
goto err;
}
+ if (!privileged) {
+ error = EPERM;
+ goto err;
+ }
+
switch (cmd) {
case SIOCSIFDSTADDR:
- if (!privileged) {
- error = EPERM;
- break;
- }
-
if ((ifp->if_flags & IFF_POINTOPOINT) == 0) {
error = EINVAL;
break;
@@ -308,11 +311,6 @@ in_ioctl(u_long cmd, caddr_t data, struc
break;
case SIOCSIFBRDADDR:
- if (!privileged) {
- error = EPERM;
- break;
- }
-
if ((ifp->if_flags & IFF_BROADCAST) == 0) {
error = EINVAL;
break;
@@ -324,11 +322,6 @@ in_ioctl(u_long cmd, caddr_t data, struc
break;
case SIOCSIFNETMASK:
- if (!privileged) {
- error = EPERM;
- break;
- }
-
if (ifr->ifr_addr.sa_len < 8) {
error = EINVAL;
break;
@@ -352,8 +345,7 @@ err:
}
int
-in_ioctl_set_ifaddr(u_long cmd, caddr_t data, struct ifnet *ifp,
-int privileged)
+in_ioctl_set_ifaddr(u_long cmd, caddr_t data, struct ifnet *ifp)
{
struct ifreq *ifr = (struct ifreq *)data;
struct ifaddr *ifa;
@@ -365,9 +357,6 @@ in_ioctl_set_ifaddr(u_long cmd, caddr_t
if (cmd != SIOCSIFADDR)
panic("%s: invalid ioctl %lu", __func__, cmd);
- if (!privileged)
- return (EPERM);
-
error = in_sa2sin(>ifr_addr, );
if (error)
return (error);
@@ -412,8 +401,7 @@ in_ioctl_set_ifaddr(u_long cmd, caddr_t
}
int
-in_ioctl_change_ifaddr(u_long cmd, caddr_t data, struct ifnet *ifp,
-int privileged)
+in_ioctl_change_ifaddr(u_long cmd, caddr_t data, struct ifnet *ifp)
{
struct ifaddr *ifa;
struct in_ifaddr *ia = NULL;
@@ -447,11 +435,6 @@ in_ioctl_change_ifaddr(u_long cmd, caddr
case SIOCAIFADDR: {
int needinit = 0;
- if (!privileged) {
- error = EPERM;
- break;
- }
-
if (ifra->ifra_mask.sin_len) {
unwind(8): fix (some?) bad packet log messages
Turns out I found a way to trigger "bad packet: too short" messages by
suspending / resuming my laptop in just the right spot. That allowed me
to figure out what's going on:
When asr fails (i.e. the stub strategy) it obviously does not synthesize
a SERVFAIL DNS packet for us like libunbound does. resolve_done() and
check_resolver_done() however depended on this. Since we are not looking
at the packet in case of SERVFAIL there is no need to check the packet
size and then complain about this, we can just bail out early.
So we first need to fix that asr errors are propagated up as rcode
SERVFAIL and then use the rcode as the first check in resolve_done() /
check_resolver_done().
I'm not sure where the "too long" errors are coming from, I suspect
there is an error path in libunbound that reports a wrong length because
it doesn't reset the internal sldns_buffer. I'm cautiously optimistic
that those errors are also caught by this diff because rcode might just
be set correctly.
OK?
commit c25ea4620c5ed21a5d12556fafba1f1ac22842e3
Author: Florian Obser
Date: Sat Apr 15 11:41:42 2023 +0200
Improve asr error handling.
When an upstream nameserver is not available asr is not synthesizing a
SERVFAIL rcode (duh), but sets ar_errno. When we need SERVFAIL we need
to set it ourselves.
While here, don't complain about a too short packet when asr already
told us that resolving did not work out in check_dns64_done.
diff --git resolver.c resolver.c
index 71614237506..9b60445f80d 100644
--- resolver.c
+++ resolver.c
@@ -1623,8 +1623,9 @@ void
asr_resolve_done(struct asr_result *ar, void *arg)
{
struct resolver_cb_data *cb_data = arg;
- cb_data->cb(cb_data->res, cb_data->data, ar->ar_rcode, ar->ar_data,
- ar->ar_datalen, 0, NULL);
+ cb_data->cb(cb_data->res, cb_data->data, ar->ar_errno == 0 ?
+ ar->ar_rcode : LDNS_RCODE_SERVFAIL, ar->ar_data, ar->ar_datalen, 0,
+ NULL);
free(ar->ar_data);
resolver_unref(cb_data->res);
free(cb_data);
@@ -2289,6 +2290,9 @@ check_dns64_done(struct asr_result *ar, void *arg)
int preflen, count = 0;
void*asr_ctx = arg;
+ if (ar->ar_errno != 0)
+ goto fail;
+
memset(, 0, sizeof(qinfo));
alloc_init(, NULL, 0);
@@ -2390,6 +2394,7 @@ check_dns64_done(struct asr_result *ar, void *arg)
alloc_clear();
regional_destroy(region);
sldns_buffer_free(buf);
+ fail:
free(ar->ar_data);
asr_resolver_free(asr_ctx);
}
commit 9ede59d6a547b0e5f574819bb7fcaf68eda24630
Author: Florian Obser
Date: Sat Apr 15 11:46:40 2023 +0200
If rcode is SERVFAIL, there is no need to look at the packet.
This pulls the check for rcode up, before we check if the answer
packet has sensible length. Since we are not touching the packet at
all, we don't care about the size and don't need to log if the size is
wrong from a DNS perspective.
With asr error reporting improved in the previous commit, this
probably gets rid of all "bad packet: too short" messages.
diff --git resolver.c resolver.c
index 9b60445f80d..9625dcb471a 100644
--- resolver.c
+++ resolver.c
@@ -953,6 +953,12 @@ resolve_done(struct uw_resolver *res, void *arg, int rcode,
running_res = --rq->running;
+ if (rcode == LDNS_RCODE_SERVFAIL) {
+ if (res->stop != 1)
+ check_resolver(res);
+ goto servfail;
+ }
+
if (answer_len < LDNS_HEADER_SIZE) {
log_warnx("bad packet: too short");
goto servfail;
@@ -965,12 +971,6 @@ resolve_done(struct uw_resolver *res, void *arg, int rcode,
}
answer_header->answer_len = answer_len;
- if (rcode == LDNS_RCODE_SERVFAIL) {
- if (res->stop != 1)
- check_resolver(res);
- goto servfail;
- }
-
if ((result = calloc(1, sizeof(*result))) == NULL)
goto servfail;
if ((buf = sldns_buffer_new(answer_len)) == NULL)
@@ -1545,12 +1545,6 @@ check_resolver_done(struct uw_resolver *res, void *arg,
int rcode,
prev_state = checked_resolver->state;
- if (answer_len < LDNS_HEADER_SIZE) {
- checked_resolver->state = DEAD;
- log_warnx("%s: bad packet: too short", __func__);
- goto out;
- }
-
if (rcode == LDNS_RCODE_SERVFAIL) {
log_debug("%s: %s rcode: SERVFAIL", __func__,
uw_resolver_type_str[checked_resolver->type]);
@@ -1559,6 +1553,12 @@ check_resolver_done(struct uw_resolver *res, void *arg,
int rcode,
goto out;
}
+ if (answer_len < LDNS_HEADER_SIZE) {
+ checked_resolver->state = DEAD;
+ log_warnx("%s: bad packet: too short", __func__);
+ goto out;
+ }
Re: MALLOC_STATS: dump internal state and leak info via utrace(2)
On Thu, Apr 13, 2023 at 08:22:45PM +0200, Otto Moerbeek wrote: > On Tue, Apr 11, 2023 at 05:50:43PM +0200, Otto Moerbeek wrote: > > > On Sun, Apr 09, 2023 at 12:17:35PM +0200, Otto Moerbeek wrote: > > > > > On Sun, Apr 09, 2023 at 10:08:25AM +0200, Claudio Jeker wrote: > > > > > > > On Sun, Apr 09, 2023 at 09:15:12AM +0200, Otto Moerbeek wrote: > > > > > On Sun, Apr 09, 2023 at 08:20:43AM +0200, Otto Moerbeek wrote: > > > > > > > > > > > On Sun, Apr 09, 2023 at 07:53:31AM +0200, Sebastien Marie wrote: > > > > > > > > > > > > > On Fri, Apr 07, 2023 at 09:52:52AM +0200, Otto Moerbeek wrote: > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > This is work in progress. I have to think if the flags to > > > > > > > > > kdump I'm > > > > > > > > > introducing should be two or a single one. > > > > > > > > > > > > > > > > > > Currently, malloc.c can be compiled with MALLOC_STATS > > > > > > > > > defined. If run > > > > > > > > > with option D it dumps its state to a malloc.out file at > > > > > > > > > exit. This > > > > > > > > > state can be used to find leaks amongst other things. > > > > > > > > > > > > > > > > > > This is not ideal for pledged processes, as they often have > > > > > > > > > no way to > > > > > > > > > write files. > > > > > > > > > > > > > > > > > > This changes malloc to use utrace(2) for that. > > > > > > > > > > > > > > > > > > As kdump has no nice way to show those lines without all > > > > > > > > > extras it > > > > > > > > > normally shows, so add two options to it to just show the > > > > > > > > > lines. > > > > > > > > > > > > > > > > > > To use, compile and install libc with MALLOC_STATS defined. > > > > > > > > > > > > > > > > > > Run : > > > > > > > > > > > > > > > > > > $ MALLOC_OPTIONS=D ktrace -tu your_program > > > > > > > > > ... > > > > > > > > > $ kdump -hu > > > > > > > > > > > > > > > > > > Feedback appreciated. > > > > > > > > > > > > > > I can't really comment on malloc(3) stuff, but I agree that > > > > > > > utrace(2) is a good > > > > > > > way to get information outside a pledged process. > > > > > > > > > > > > > > I tend to think it is safe to use it, as the pledged process need > > > > > > > cooperation > > > > > > > from outside to exfiltrate informations (a process with > > > > > > > permission to call > > > > > > > ktrace(2) on this pid). > > > > > > > > > > > > > > Please note it is a somehow generic problem: at least profiled > > > > > > > processes would > > > > > > > also get advantage of using it. > > > > > > > > > > > > > > > > > > > > > Regarding kdump options, I think that -u option should implies -h > > > > > > > (no header). > > > > > > > > > > > > > > Does it would make sens to considere a process using utrace(2) > > > > > > > with several > > > > > > > interleaved records for different sources ? A process with > > > > > > > MALLOC_OPTIONS=D and > > > > > > > profiling enabled for example ? An (another) option on kdump to > > > > > > > filter on utrace > > > > > > > label would be useful in such case, or have -u mandate a label to > > > > > > > filter on. > > > > > > > > > > > > > > $ MALLOC_OPTIONS=D ktrace -tu your_program > > > > > > > $ kdump -u mallocdumpline > > > > > > > > > > > > > > and for profiling: > > > > > > > > > > > > > > $ kdump -u profil > gmon.out > > > > > > > $ gprof your_program gmon.out > > > > > > > > > > > > > > Thanks. > > > > > > > > > > > > Thanks! Your suggestions make a lot of sense. I'll rework the kdump > > > > > > part to make it more flexable for different purposes. > > > > > > > > > > Anothew aspect of safety is the information availble in the heap > > > > > itself. I'm pretty sure the addresses of call sites into malloc are > > > > > interesting to attackers. To prevent a program having access to those > > > > > (even if they are stored inside the malloc meta data that is not > > > > > directly accesible to a program), I'm making sure the recording only > > > > > takes place if malloc option D is used. > > > > > > > > > > This is included in a diff with the kdump changes and a few other > > > > > things below. I'm also compiling with MALLOC_STATS if !SMALL. > > > > > > > > > > usage is now: > > > > > > > > > > $ MALLOC_OPTIONS=D ktrace -tu a.out > > > > > $ kdump -u malloc > > > > > > > > > > > > > I would prefer if every utrace() call is a full line (in other words > > > > ulog > > > > should be line buffered). It makes the regular kdump output more > > > > useable. > > > > Right now you depend on kdump -u to put the lines back together. > > > > > > Right. Done line buffering in the new diff below. > > > > > > > Whenever I used utrace() I normally passed binary objects to the call > > > > so I > > > > could enrich the ktrace with userland trace info. So if kdump -u is > > > > used > > > > for more then just mallocstats it should have a true binary mode. For > > > > example gmon.out is a binary format so the above example by semarie@ > > > >
Re: pf(4) drops valid IGMP/MLD messages
I've just seen that this has been fixed in 7.3, now I see that prune, graft, graft-ack messages are not blocked by PF Thanks a lot Il giorno gio 16 mar 2023 alle ore 16:45 Luca Di Gregorio ha scritto: > Ok, thanks a lot for the info > > Il giorno gio 16 mar 2023 alle 16:40 Theo de Raadt > ha scritto: > >> Luca Di Gregorio wrote: >> >> > do you think that the correction proposed by Alexandr could be done >> with a >> > syspatch, or in the next release? >> >> It does not meet the treshold for becoming a syspatch. >> >
bgpctl command parser any token support
In bgpctl I am constantly hitting this annoying edgecase where adding
detail to a show rib command errors out:
bgpctl show rib 192.0.2.1 detail
unknown argument: detail
valid commands/args:
all
longer-prefixes
or-longer
or-shorter
The problem is that the t_show_prefix token table can not fall back to
t_show_rib if an unknown token is seen.
This diff solves this issue with the introduction of ANYTOKEN.
ANYTOKEN matches if nothing previously matched and then redirects
the call to the next table (which is pointing back up).
To make show_valid_args() work nicely dump store the first table on
ANYTOKEN and dump the help for that one first. I don't think there is a
need to have a stack of tables since ANYTOKEN should be used sparingly.
With this 'bgpctl show rib 192.0.2.1 detail' works.
--
:wq Claudio
Index: parser.c
===
RCS file: /cvs/src/usr.sbin/bgpctl/parser.c,v
retrieving revision 1.124
diff -u -p -r1.124 parser.c
--- parser.c13 Apr 2023 11:52:43 - 1.124
+++ parser.c15 Apr 2023 08:34:25 -
@@ -34,8 +34,9 @@
#include "parser.h"
enum token_type {
- NOTOKEN,
ENDTOKEN,
+ NOTOKEN,
+ ANYTOKEN,
KEYWORD,
ADDRESS,
PEERADDRESS,
@@ -72,6 +73,8 @@ struct token {
const struct token *next;
};
+static const struct token *prevtable;
+
static const struct token t_main[];
static const struct token t_show[];
static const struct token t_show_summary[];
@@ -320,11 +323,11 @@ static const struct token t_show_mrt_as[
};
static const struct token t_show_prefix[] = {
- { NOTOKEN, "", NONE, NULL},
- { FLAG, "all", F_LONGER, NULL},
- { FLAG, "longer-prefixes", F_LONGER,NULL},
- { FLAG, "or-longer",F_LONGER, NULL},
- { FLAG, "or-shorter", F_SHORTER, NULL},
+ { FLAG, "all", F_LONGER, t_show_rib},
+ { FLAG, "longer-prefixes", F_LONGER,t_show_rib},
+ { FLAG, "or-longer",F_LONGER, t_show_rib},
+ { FLAG, "or-shorter", F_SHORTER, t_show_rib},
+ { ANYTOKEN, "", NONE, t_show_rib},
{ ENDTOKEN, "", NONE, NULL}
};
@@ -533,6 +536,12 @@ parse(int argc, char *argv[])
show_valid_args(table);
return (NULL);
}
+ if (match->type == ANYTOKEN) {
+ if (prevtable == NULL)
+ prevtable = table;
+ table = match->next;
+ continue;
+ }
argc--;
argv++;
@@ -571,6 +580,13 @@ match_token(int *argc, char **argv[], co
t = [i];
}
break;
+ case ANYTOKEN:
+ /* match anything if nothing else matched before */
+ if (match == 0) {
+ match++;
+ t = [i];
+ }
+ break;
case KEYWORD:
if (word != NULL && strncmp(word, table[i].keyword,
wordlen) == 0) {
@@ -842,10 +858,19 @@ show_valid_args(const struct token table
{
int i;
+ if (prevtable != NULL) {
+ const struct token *t = prevtable;
+ prevtable = NULL;
+ show_valid_args(t);
+ fprintf(stderr, "or any of\n");
+ }
+
for (i = 0; table[i].type != ENDTOKEN; i++) {
switch (table[i].type) {
case NOTOKEN:
fprintf(stderr, " \n");
+ break;
+ case ANYTOKEN:
break;
case KEYWORD:
case FLAG:
