Re: rpki-client: plug leak in http_parse_header()
On Thu, Feb 10, 2022 at 07:51:45AM +0100, Theo Buehler wrote:
> At this point conn->last_modified may or may not be allocated.
> If it is, overriting it will leak 30 bytes.
rrdp_input_handler() has a leak of the same kind.
Index: http.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/http.c,v
retrieving revision 1.52
diff -u -p -r1.52 http.c
--- http.c 23 Jan 2022 12:09:24 - 1.52
+++ http.c 10 Feb 2022 02:09:38 -
@@ -1231,6 +1231,7 @@ http_parse_header(struct http_connection
} else if (strncasecmp(cp, LAST_MODIFIED,
sizeof(LAST_MODIFIED) - 1) == 0) {
cp += sizeof(LAST_MODIFIED) - 1;
+ free(conn->last_modified);
if ((conn->last_modified = strdup(cp)) == NULL)
err(1, NULL);
}
Index: rrdp.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/rrdp.c,v
retrieving revision 1.21
diff -u -p -r1.21 rrdp.c
--- rrdp.c 23 Jan 2022 12:09:24 - 1.21
+++ rrdp.c 10 Feb 2022 07:41:54 -
@@ -429,6 +429,7 @@ rrdp_input_handler(int fd)
errx(1, "%s: bad internal state", s->local);
s->res = res;
+ free(s->last_mod);
s->last_mod = last_mod;
s->state |= RRDP_STATE_HTTP_DONE;
rrdp_finished(s);
Re: fw_update(8): lock pkg database while running
On Wed, Feb 09, 2022 at 07:30:46PM -0800, Andrew Hewus Fresh wrote:
> I was reminded that fw_update(8) updates the package database without
> locking currently. That can cause issues when running it concurrently
> with pkg_add, for example starting `pkg_add -u` in one terminal and
> `sysupgrade` in another.
>
> This diff checks to see if perl is available and if so starts a perl
> co-process that locks the database and then kills it when exiting.
>
> (Figuring out how to wait for the lock to be acquired and find out the
> pid of the co-process was a bit of a challenge, so if someone has a
> suggestion on improving that, I'm open to suggestions)
>
> Comments, OK?
>
>
> Index: usr.sbin/fw_update/fw_update.sh
> ===
> RCS file: /cvs/src/usr.sbin/fw_update/fw_update.sh,v
> retrieving revision 1.36
> diff -u -p -r1.36 fw_update.sh
> --- usr.sbin/fw_update/fw_update.sh 10 Feb 2022 00:29:32 - 1.36
> +++ usr.sbin/fw_update/fw_update.sh 10 Feb 2022 03:22:20 -
> @@ -42,11 +42,13 @@ INSTALL=true
> LOCALSRC=
>
> unset FTPPID
> +unset LOCKPID
> unset FWPKGTMP
> REMOVE_LOCALSRC=false
> cleanup() {
> set +o errexit # ignore errors from killing ftp
> [ "${FTPPID:-}" ] && kill -TERM -"$FTPPID" 2>/dev/null
> + [ "${LOCKPID:-}" ] && kill -TERM -"$LOCKPID" 2>/dev/null
> [ "${FWPKGTMP:-}" ] && rm -rf "$FWPKGTMP"
> "$REMOVE_LOCALSRC" && rm -rf "$LOCALSRC"
> [ -e "${CFILE}" ] && [ ! -s "$CFILE" ] && rm -f "$CFILE"
> @@ -194,6 +196,36 @@ firmware_devicename() {
> echo "$_d"
> }
>
> +lock_db() {
> + [ "${LOCKPID:-}" ] && return 0
> +
> + # The installer doesn't have perl, so we can't lock there
> + [ -e /usr/bin/perl ] || return 0
> +
> + set -o monitor
> + perl <<'EOL' |&
> + use v5.16;
> + use warnings;
> + use OpenBSD::PackageInfo qw< lock_db unlock_db >;
> + use OpenBSD::State;
> +
> + $|=1;
> +
> + lock_db(0, OpenBSD::State->new);
> + END { unlock_db }
> + $SIG{TERM} = sub { exit };
> +
> + say $$;
> + sleep;
> +EOL
> + set +o monitor
> +
> + read -rp LOCKPID
> +
> + return 0
> +}
> +
> +
> installed_firmware() {
> local _pre="$1" _match="$2" _post="$3" _firmware _fw
> set -sA _firmware -- $(
> @@ -401,6 +433,7 @@ set -sA devices -- "$@"
>
> if "$DELETE"; then
> [ "$OPT_F" ] && echo "Cannot use -F and -d" >&2 && usage
> + lock_db
>
> # Show the "Uninstall" message when just deleting not upgrading
> ((VERBOSE)) && VERBOSE=3
> @@ -463,6 +496,8 @@ else
> fi
>
> [ "${devices[*]:-}" ] || exit
> +
> +lock_db
>
> added=''
> updated=''
>
It's one of the cases where you can actually use the newer OpenBSD::BaseState
where I explicitly split the printing and system functions.
e.g.,
use OpenBSD::BaseState;
lock_db(0, 'OpenBSD::BaseState');
should work just as well
rpki-client: plug leak in http_parse_header()
At this point conn->last_modified may or may not be allocated.
If it is, overriting it will leak 30 bytes.
Index: http.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/http.c,v
retrieving revision 1.52
diff -u -p -r1.52 http.c
--- http.c 23 Jan 2022 12:09:24 - 1.52
+++ http.c 10 Feb 2022 02:09:38 -
@@ -1231,6 +1231,7 @@ http_parse_header(struct http_connection
} else if (strncasecmp(cp, LAST_MODIFIED,
sizeof(LAST_MODIFIED) - 1) == 0) {
cp += sizeof(LAST_MODIFIED) - 1;
+ free(conn->last_modified);
if ((conn->last_modified = strdup(cp)) == NULL)
err(1, NULL);
}
fw_update(8): lock pkg database while running
I was reminded that fw_update(8) updates the package database without
locking currently. That can cause issues when running it concurrently
with pkg_add, for example starting `pkg_add -u` in one terminal and
`sysupgrade` in another.
This diff checks to see if perl is available and if so starts a perl
co-process that locks the database and then kills it when exiting.
(Figuring out how to wait for the lock to be acquired and find out the
pid of the co-process was a bit of a challenge, so if someone has a
suggestion on improving that, I'm open to suggestions)
Comments, OK?
Index: usr.sbin/fw_update/fw_update.sh
===
RCS file: /cvs/src/usr.sbin/fw_update/fw_update.sh,v
retrieving revision 1.36
diff -u -p -r1.36 fw_update.sh
--- usr.sbin/fw_update/fw_update.sh 10 Feb 2022 00:29:32 - 1.36
+++ usr.sbin/fw_update/fw_update.sh 10 Feb 2022 03:22:20 -
@@ -42,11 +42,13 @@ INSTALL=true
LOCALSRC=
unset FTPPID
+unset LOCKPID
unset FWPKGTMP
REMOVE_LOCALSRC=false
cleanup() {
set +o errexit # ignore errors from killing ftp
[ "${FTPPID:-}" ] && kill -TERM -"$FTPPID" 2>/dev/null
+ [ "${LOCKPID:-}" ] && kill -TERM -"$LOCKPID" 2>/dev/null
[ "${FWPKGTMP:-}" ] && rm -rf "$FWPKGTMP"
"$REMOVE_LOCALSRC" && rm -rf "$LOCALSRC"
[ -e "${CFILE}" ] && [ ! -s "$CFILE" ] && rm -f "$CFILE"
@@ -194,6 +196,36 @@ firmware_devicename() {
echo "$_d"
}
+lock_db() {
+ [ "${LOCKPID:-}" ] && return 0
+
+ # The installer doesn't have perl, so we can't lock there
+ [ -e /usr/bin/perl ] || return 0
+
+ set -o monitor
+ perl <<'EOL' |&
+ use v5.16;
+ use warnings;
+ use OpenBSD::PackageInfo qw< lock_db unlock_db >;
+ use OpenBSD::State;
+
+ $|=1;
+
+ lock_db(0, OpenBSD::State->new);
+ END { unlock_db }
+ $SIG{TERM} = sub { exit };
+
+ say $$;
+ sleep;
+EOL
+ set +o monitor
+
+ read -rp LOCKPID
+
+ return 0
+}
+
+
installed_firmware() {
local _pre="$1" _match="$2" _post="$3" _firmware _fw
set -sA _firmware -- $(
@@ -401,6 +433,7 @@ set -sA devices -- "$@"
if "$DELETE"; then
[ "$OPT_F" ] && echo "Cannot use -F and -d" >&2 && usage
+ lock_db
# Show the "Uninstall" message when just deleting not upgrading
((VERBOSE)) && VERBOSE=3
@@ -463,6 +496,8 @@ else
fi
[ "${devices[*]:-}" ] || exit
+
+lock_db
added=''
updated=''
Re: rpki-client: check crl validity times
On Wed, Feb 09, 2022 at 02:59:41PM +0100, Theo Buehler wrote:
> We should not use CRLs if now isn't between thisUpdate and nextUpdate.
> This also ensures that thisUpdate <= nextUpdate. While the verifier will
> catch all this, doing this early will often remove one of the two
> possible choices of a CRL to use for a MFT since these are typically
> short-lived. While there, let's simplify the exit of crl_parse().
>
> I was pondering whether we should mark such CRLs stale and add them to
> the statistics as we do for MFTs, but I think it's not super
> interesting.
I'm not fully convinced by this. Mainly not returning a CRL will alter the
error reported by X509_verify_cert() and make it more confusing
(especially since the warnings in crl_parse are only if verbose > 1.
I would not mind to do this check in parse_load_crl_from_mft().
Another thing we should consider is that the CRL used to validate the MFT
should also be the one used to validate the rest. This is currently not
enforced.
Will need to think about this more.
> Index: crl.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/crl.c,v
> retrieving revision 1.13
> diff -u -p -r1.13 crl.c
> --- crl.c 8 Feb 2022 14:53:03 - 1.13
> +++ crl.c 9 Feb 2022 06:23:30 -
> @@ -34,7 +34,7 @@ crl_parse(const char *fn, const unsigned
> struct crl *crl;
> const ASN1_TIME *at;
> struct tmissued_tm, expires_tm;
> - int rc = 0;
> + time_t now;
>
> /* just fail for empty buffers, the warning was printed elsewhere */
> if (der == NULL)
> @@ -66,7 +66,6 @@ crl_parse(const char *fn, const unsigned
> if ((crl->issued = mktime(_tm)) == -1)
> errx(1, "%s: mktime failed", fn);
>
> - /* extract expire time for later use */
> at = X509_CRL_get0_nextUpdate(crl->x509_crl);
> if (at == NULL) {
> warnx("%s: X509_CRL_get0_nextUpdate failed", fn);
> @@ -80,13 +79,25 @@ crl_parse(const char *fn, const unsigned
> if ((crl->expires = mktime(_tm)) == -1)
> errx(1, "%s: mktime failed", fn);
>
> - rc = 1;
> - out:
> - if (rc == 0) {
> - crl_free(crl);
> - crl = NULL;
> + now = time(NULL);
> + if (now < crl->issued) {
> + if (verbose > 1)
> + warnx("%s: crl not yet valid %s", fn,
> + time2str(crl->issued));
> + goto out;
> + }
> + if (now > crl->expires) {
> + if (verbose > 1)
> + warnx("%s: crl expired on %s", fn,
> + time2str(crl->expires));
> + goto out;
> }
> +
> return crl;
> +
> + out:
> + crl_free(crl);
> + return NULL;
> }
>
> static inline int
> Index: extern.h
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.118
> diff -u -p -r1.118 extern.h
> --- extern.h 8 Feb 2022 14:53:03 - 1.118
> +++ extern.h 9 Feb 2022 06:21:49 -
> @@ -502,6 +502,7 @@ void entity_free(struct entity *);
> void entity_read_req(struct ibuf *, struct entity *);
> void entityq_flush(struct entityq *, struct repo *);
> void proc_parser(int) __attribute__((noreturn));
> +char *time2str(time_t);
>
> /* Rsync-specific. */
>
> Index: parser.c
> ===
> RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
> retrieving revision 1.63
> diff -u -p -r1.63 parser.c
> --- parser.c 8 Feb 2022 14:53:03 - 1.63
> +++ parser.c 9 Feb 2022 06:19:40 -
> @@ -94,7 +94,7 @@ repo_add(unsigned int id, char *path, ch
> errx(1, "repository already added: id %d, %s", id, path);
> }
>
> -static char *
> +char *
> time2str(time_t t)
> {
> static char buf[64];
>
--
:wq Claudio
Re: explicit_bzero vs ASAN on linux
On Wed, 09 Feb 2022 11:35:58 +0100, Theo Buehler wrote: > On clang we can use __has_feature(), but that doesn't exist on > gcc which defines __SANITIZE_ADDRESS__ if it compiles with > -fsanitize=address. > > This doesn't warn on sparc64 and works in my test setups. It's a little ugly but since this only affects a regress test I think it is OK. This may help us in the future if we gain address sanitizer for our clang. - todd
Re: look(1): drop "rpath" promise after open(2)/fstat(2)
On Tue, 08 Feb 2022 19:37:26 -0600, Scott Cheloha wrote: > - pledge(2) initially with "stdio rpath" at the top of main(). > We know we need to read a file at this point but don't yet > know which one. > > - pledge(2) down to "stdio" after we have opened the file > in question and called fstat(2) to get its size. The rest > of the program is computation and stdio. > > - Remove the unveil(2) call. We don't need it if we're only > working with one file and it's already open. Makes sense to me. I don't think the unveil() really buys us anything if we can drop rpath. OK millert@ - todd
Re: add Surface GO3 hid to acpibat(4)
> From: Dave Voutila
> Date: Wed, 09 Feb 2022 10:28:48 -0500
>
> I believe I got this from mlarkin@. Realized I've been carrying it in my
> local tree.
>
> Microsoft decided to do all sorts of atypical things with the Go3 in
> ACPI. One is the battery.
>
> ok?
Funny how even Microsoft manages to turn ACPI into a dog's breakfast.
Would have expected they'd have a
Name (_CID, EidaId ("PNP0C0A"))
in there if the interface is compatible with the ACPI standard Control
Method Battery interface. But no...
ok kettenis@
> diff 555fd15dcf830b9fb7a50490d9996f605a238ab5
> 12b5e3cfa4623c60f84e61950df48b71cf2d8ef0
> blob - 9075aa39722d9551251930287341657a41a0db14
> blob + 321481d55e086d42373e62ebe181a9deb492fadf
> --- sys/dev/acpi/acpibat.c
> +++ sys/dev/acpi/acpibat.c
> @@ -47,6 +47,7 @@ struct cfdriver acpibat_cd = {
>
> const char *acpibat_hids[] = {
> ACPI_DEV_CMB,
> + "MSHW0146",
> NULL
> };
>
>
add Surface GO3 hid to acpibat(4)
I believe I got this from mlarkin@. Realized I've been carrying it in my
local tree.
Microsoft decided to do all sorts of atypical things with the Go3 in
ACPI. One is the battery.
ok?
-dv
diff 555fd15dcf830b9fb7a50490d9996f605a238ab5
12b5e3cfa4623c60f84e61950df48b71cf2d8ef0
blob - 9075aa39722d9551251930287341657a41a0db14
blob + 321481d55e086d42373e62ebe181a9deb492fadf
--- sys/dev/acpi/acpibat.c
+++ sys/dev/acpi/acpibat.c
@@ -47,6 +47,7 @@ struct cfdriver acpibat_cd = {
const char *acpibat_hids[] = {
ACPI_DEV_CMB,
+ "MSHW0146",
NULL
};
Re: Embed klist head in acpi_softc
> Date: Wed, 9 Feb 2022 15:24:19 +
> From: Visa Hankala
>
> This embeds klist head in struct acpi_softc so that explicit malloc is
> not needed. The head is initialized as part of acpi_softc allocation.
>
> OK?
ok kettenis@
> Index: dev/acpi/acpi.c
> ===
> RCS file: src/sys/dev/acpi/acpi.c,v
> retrieving revision 1.410
> diff -u -p -r1.410 acpi.c
> --- dev/acpi/acpi.c 8 Feb 2022 17:25:12 - 1.410
> +++ dev/acpi/acpi.c 9 Feb 2022 15:12:22 -
> @@ -1016,16 +1016,6 @@ acpi_attach_common(struct acpi_softc *sc
> SIMPLEQ_INIT(>sc_pwrresdevs);
> #endif /* NACPIPWRRES > 0 */
>
> -
> -#ifndef SMALL_KERNEL
> - sc->sc_note = malloc(sizeof(struct klist), M_DEVBUF, M_NOWAIT | M_ZERO);
> - if (sc->sc_note == NULL) {
> - printf(": can't allocate memory\n");
> - acpi_unmap();
> - return;
> - }
> -#endif /* SMALL_KERNEL */
> -
> if (acpi_loadtables(sc, rsdp)) {
> printf(": can't load tables\n");
> acpi_unmap();
> @@ -3467,7 +3457,7 @@ acpi_record_event(struct acpi_softc *sc,
> return (1);
>
> acpi_evindex++;
> - KNOTE(sc->sc_note, APM_EVENT_COMPOSE(type, acpi_evindex));
> + KNOTE(>sc_note, APM_EVENT_COMPOSE(type, acpi_evindex));
> return (0);
> }
>
> @@ -3478,7 +3468,7 @@ acpi_filtdetach(struct knote *kn)
> int s;
>
> s = splbio();
> - klist_remove_locked(sc->sc_note, kn);
> + klist_remove_locked(>sc_note, kn);
> splx(s);
> }
>
> @@ -3512,7 +3502,7 @@ acpikqfilter(dev_t dev, struct knote *kn
> kn->kn_hook = sc;
>
> s = splbio();
> - klist_insert_locked(sc->sc_note, kn);
> + klist_insert_locked(>sc_note, kn);
> splx(s);
>
> return (0);
> Index: dev/acpi/acpivar.h
> ===
> RCS file: src/sys/dev/acpi/acpivar.h,v
> retrieving revision 1.118
> diff -u -p -r1.118 acpivar.h
> --- dev/acpi/acpivar.h8 Feb 2022 17:25:12 - 1.118
> +++ dev/acpi/acpivar.h9 Feb 2022 15:12:22 -
> @@ -23,6 +23,7 @@
>
> #ifndef _ACPI_WAKECODE
>
> +#include
> #include
> #include
>
> @@ -45,7 +46,6 @@ extern int acpi_debug;
> extern int acpi_hasprocfvs;
> extern int acpi_haspci;
>
> -struct klist;
> struct acpiec_softc;
> struct acpipwrres_softc;
>
> @@ -239,7 +239,7 @@ struct acpi_softc {
>*/
> struct acpi_facs*sc_facs; /* Shared with firmware! */
>
> - struct klist*sc_note;
> + struct klistsc_note;
> struct acpi_reg_map sc_pmregs[ACPIREG_MAXREG];
> bus_space_handle_t sc_ioh_pm1a_evt;
>
>
>
Embed klist head in acpi_softc
This embeds klist head in struct acpi_softc so that explicit malloc is
not needed. The head is initialized as part of acpi_softc allocation.
OK?
Index: dev/acpi/acpi.c
===
RCS file: src/sys/dev/acpi/acpi.c,v
retrieving revision 1.410
diff -u -p -r1.410 acpi.c
--- dev/acpi/acpi.c 8 Feb 2022 17:25:12 - 1.410
+++ dev/acpi/acpi.c 9 Feb 2022 15:12:22 -
@@ -1016,16 +1016,6 @@ acpi_attach_common(struct acpi_softc *sc
SIMPLEQ_INIT(>sc_pwrresdevs);
#endif /* NACPIPWRRES > 0 */
-
-#ifndef SMALL_KERNEL
- sc->sc_note = malloc(sizeof(struct klist), M_DEVBUF, M_NOWAIT | M_ZERO);
- if (sc->sc_note == NULL) {
- printf(": can't allocate memory\n");
- acpi_unmap();
- return;
- }
-#endif /* SMALL_KERNEL */
-
if (acpi_loadtables(sc, rsdp)) {
printf(": can't load tables\n");
acpi_unmap();
@@ -3467,7 +3457,7 @@ acpi_record_event(struct acpi_softc *sc,
return (1);
acpi_evindex++;
- KNOTE(sc->sc_note, APM_EVENT_COMPOSE(type, acpi_evindex));
+ KNOTE(>sc_note, APM_EVENT_COMPOSE(type, acpi_evindex));
return (0);
}
@@ -3478,7 +3468,7 @@ acpi_filtdetach(struct knote *kn)
int s;
s = splbio();
- klist_remove_locked(sc->sc_note, kn);
+ klist_remove_locked(>sc_note, kn);
splx(s);
}
@@ -3512,7 +3502,7 @@ acpikqfilter(dev_t dev, struct knote *kn
kn->kn_hook = sc;
s = splbio();
- klist_insert_locked(sc->sc_note, kn);
+ klist_insert_locked(>sc_note, kn);
splx(s);
return (0);
Index: dev/acpi/acpivar.h
===
RCS file: src/sys/dev/acpi/acpivar.h,v
retrieving revision 1.118
diff -u -p -r1.118 acpivar.h
--- dev/acpi/acpivar.h 8 Feb 2022 17:25:12 - 1.118
+++ dev/acpi/acpivar.h 9 Feb 2022 15:12:22 -
@@ -23,6 +23,7 @@
#ifndef _ACPI_WAKECODE
+#include
#include
#include
@@ -45,7 +46,6 @@ extern int acpi_debug;
extern int acpi_hasprocfvs;
extern int acpi_haspci;
-struct klist;
struct acpiec_softc;
struct acpipwrres_softc;
@@ -239,7 +239,7 @@ struct acpi_softc {
*/
struct acpi_facs*sc_facs; /* Shared with firmware! */
- struct klist*sc_note;
+ struct klistsc_note;
struct acpi_reg_map sc_pmregs[ACPIREG_MAXREG];
bus_space_handle_t sc_ioh_pm1a_evt;
rpki-client: check crl validity times
We should not use CRLs if now isn't between thisUpdate and nextUpdate.
This also ensures that thisUpdate <= nextUpdate. While the verifier will
catch all this, doing this early will often remove one of the two
possible choices of a CRL to use for a MFT since these are typically
short-lived. While there, let's simplify the exit of crl_parse().
I was pondering whether we should mark such CRLs stale and add them to
the statistics as we do for MFTs, but I think it's not super
interesting.
Index: crl.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/crl.c,v
retrieving revision 1.13
diff -u -p -r1.13 crl.c
--- crl.c 8 Feb 2022 14:53:03 - 1.13
+++ crl.c 9 Feb 2022 06:23:30 -
@@ -34,7 +34,7 @@ crl_parse(const char *fn, const unsigned
struct crl *crl;
const ASN1_TIME *at;
struct tmissued_tm, expires_tm;
- int rc = 0;
+ time_t now;
/* just fail for empty buffers, the warning was printed elsewhere */
if (der == NULL)
@@ -66,7 +66,6 @@ crl_parse(const char *fn, const unsigned
if ((crl->issued = mktime(_tm)) == -1)
errx(1, "%s: mktime failed", fn);
- /* extract expire time for later use */
at = X509_CRL_get0_nextUpdate(crl->x509_crl);
if (at == NULL) {
warnx("%s: X509_CRL_get0_nextUpdate failed", fn);
@@ -80,13 +79,25 @@ crl_parse(const char *fn, const unsigned
if ((crl->expires = mktime(_tm)) == -1)
errx(1, "%s: mktime failed", fn);
- rc = 1;
- out:
- if (rc == 0) {
- crl_free(crl);
- crl = NULL;
+ now = time(NULL);
+ if (now < crl->issued) {
+ if (verbose > 1)
+ warnx("%s: crl not yet valid %s", fn,
+ time2str(crl->issued));
+ goto out;
+ }
+ if (now > crl->expires) {
+ if (verbose > 1)
+ warnx("%s: crl expired on %s", fn,
+ time2str(crl->expires));
+ goto out;
}
+
return crl;
+
+ out:
+ crl_free(crl);
+ return NULL;
}
static inline int
Index: extern.h
===
RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
retrieving revision 1.118
diff -u -p -r1.118 extern.h
--- extern.h8 Feb 2022 14:53:03 - 1.118
+++ extern.h9 Feb 2022 06:21:49 -
@@ -502,6 +502,7 @@ void entity_free(struct entity *);
voidentity_read_req(struct ibuf *, struct entity *);
voidentityq_flush(struct entityq *, struct repo *);
voidproc_parser(int) __attribute__((noreturn));
+char *time2str(time_t);
/* Rsync-specific. */
Index: parser.c
===
RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
retrieving revision 1.63
diff -u -p -r1.63 parser.c
--- parser.c8 Feb 2022 14:53:03 - 1.63
+++ parser.c9 Feb 2022 06:19:40 -
@@ -94,7 +94,7 @@ repo_add(unsigned int id, char *path, ch
errx(1, "repository already added: id %d, %s", id, path);
}
-static char *
+char *
time2str(time_t t)
{
static char buf[64];
Re: hardware checksum ix and ixl
On Wed, Jan 26, 2022 at 01:29:42AM +0100, Alexander Bluhm wrote:
> Hi,
>
> There were some problems with ix(4) and ixl(4) hardware checksumming
> for the output path on strict alignment architectures.
>
> I have merged jan@'s diffs and added some sanity checks and
> workarounds.
>
> - If the first mbuf is not aligned or not contigous, use m_copydata()
> to extract the IP, IPv6, TCP header.
> - If the header is in the first mbuf, use m_data for the fast path.
> - Add netstat counter for invalid header chains. This makes
> us aware when hardware checksumming fails.
> - Add netstat counter for header copies. This indicates that
> better storage allocation in the network stack is possible.
> It also allows to recognize alignment problems on non-strict
> architectures.
> - There is not risk of crashes on sparc64.
>
> Does this aproach make sense?
>
> ix(4) works quite well, but finds some UDP packets that need copy.
> ixl(4) has not been tested yet. I would like to have some feedback
> for the idea first.
>
> bluhm
>
> Index: sys/dev/pci/if_ixl.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/dev/pci/if_ixl.c,v
> retrieving revision 1.78
> diff -u -p -r1.78 if_ixl.c
> --- sys/dev/pci/if_ixl.c 9 Jan 2022 05:42:54 - 1.78
> +++ sys/dev/pci/if_ixl.c 25 Jan 2022 23:50:01 -
> @@ -1388,6 +1398,7 @@ static int ixl_rxeof(struct ixl_softc *,
> static void ixl_rxfill(struct ixl_softc *, struct ixl_rx_ring *);
> static void ixl_rxrefill(void *);
> static int ixl_rxrinfo(struct ixl_softc *, struct if_rxrinfo *);
> +static void ixl_rx_checksum(struct mbuf *, uint64_t);
>
> #if NKSTAT > 0
> static void ixl_kstat_attach(struct ixl_softc *);
> @@ -3190,6 +3318,7 @@ ixl_rxeof(struct ixl_softc *sc, struct i
> m->m_pkthdr.csum_flags |= M_FLOWID;
> }
>
> + ixl_rx_checksum(m, word);
> ml_enqueue(, m);
> } else {
> ifp->if_ierrors++; /* XXX */
> @@ -3320,6 +3449,23 @@ ixl_rxrinfo(struct ixl_softc *sc, struct
> free(ifr, M_TEMP, ixl_nqueues(sc) * sizeof(*ifr));
>
> return (rv);
> +}
> +
> +static void
> +ixl_rx_checksum(struct mbuf *m, uint64_t word)
> +{
> + if (!ISSET(word, IXL_RX_DESC_L3L4P))
> + return;
> +
> + if (ISSET(word, IXL_RX_DESC_IPE))
> + return;
> +
> + m->m_pkthdr.csum_flags |= M_IPV4_CSUM_IN_OK;
> +
> + if (ISSET(word, IXL_RX_DESC_L4E))
> + return;
> +
> + m->m_pkthdr.csum_flags |= M_TCP_CSUM_IN_OK | M_UDP_CSUM_IN_OK;
> }
>
> static int
this is ok by me. tested on both amd64 and sparc64.
dlg
Re: explicit_bzero vs ASAN on linux
On Wed, Feb 09, 2022 at 08:45:09PM +1100, Jonathan Gray wrote:
> On Wed, Feb 09, 2022 at 09:09:35AM +0100, Theo Buehler wrote:
> > In libressl-portable we run the explicit_bzero tests as part of the
> > builds. If we enable ASAN on linux, this test segfaults in
> > __interceptor_memmem() in the two test_with{,out}_bzero() functions,
> > presumably because the sigaltstack magic is too low level for ASAN to
> > grok.
> >
> > Would the patch below that disables ASAN for these functions be
> > acceptable or shold we maintain it in the libressl-portable repo?
> >
> > https://github.com/google/sanitizers/wiki/AddressSanitizer#turning-off-instrumentation
> >
> > Index: explicit_bzero.c
> > ===
> > RCS file: /cvs/src/regress/lib/libc/explicit_bzero/explicit_bzero.c,v
> > retrieving revision 1.8
> > diff -u -p -r1.8 explicit_bzero.c
> > --- explicit_bzero.c9 Feb 2022 07:48:15 - 1.8
> > +++ explicit_bzero.c9 Feb 2022 07:56:43 -
> > @@ -26,6 +26,12 @@
> > #define ASSERT_NE(a, b) assert((a) != (b))
> > #define ASSERT_GE(a, b) assert((a) >= (b))
> >
> > +#if defined(__clang__) || defined (__GNUC__)
> > +#define ATTRIBUTE_NO_SANITIZE_ADDRESS __attribute__((no_sanitize_address))
> > +#else
> > +#define ATTRIBUTE_NO_SANITIZE_ADDRESS
> > +#endif
> > +
>
> clang also defines __GNUC__, not sure if clang-cl is different
>
> with clang 13.0.0 still 4.2.1
>
> #define __GNUC__ 4
> #define __GNUC_MINOR__ 2
> #define __GNUC_PATCHLEVEL__ 1
>
> I was curious how actual gcc 4.2.1 would handle this so I tried on
> sparc64. The test still passes but there is a warning about an unknown
> attribute.
Thanks. The below is based on a suggestion by Ilya Shipitsin.
On clang we can use __has_feature(), but that doesn't exist on
gcc which defines __SANITIZE_ADDRESS__ if it compiles with
-fsanitize=address.
This doesn't warn on sparc64 and works in my test setups.
Index: explicit_bzero.c
===
RCS file: /cvs/src/regress/lib/libc/explicit_bzero/explicit_bzero.c,v
retrieving revision 1.8
diff -u -p -r1.8 explicit_bzero.c
--- explicit_bzero.c9 Feb 2022 07:48:15 - 1.8
+++ explicit_bzero.c9 Feb 2022 10:32:00 -
@@ -1,4 +1,4 @@
-/* $OpenBSD: explicit_bzero.c,v 1.8 2022/02/09 07:48:15 tb Exp $ */
+/* $OpenBSD: explicit_bzero.c,v 1.6 2014/07/11 01:10:35 matthew Exp $
*/
/*
* Copyright (c) 2014 Google Inc.
*
@@ -26,6 +26,17 @@
#define ASSERT_NE(a, b) assert((a) != (b))
#define ASSERT_GE(a, b) assert((a) >= (b))
+#if defined(__has_feature)
+#if __has_feature(address_sanitizer)
+#define __SANITIZE_ADDRESS__
+#endif
+#endif
+#ifdef __SANITIZE_ADDRESS__
+#define ATTRIBUTE_NO_SANITIZE_ADDRESS __attribute__((no_sanitize_address))
+#else
+#define ATTRIBUTE_NO_SANITIZE_ADDRESS
+#endif
+
/* 128 bits of random data. */
static const char secret[16] = {
0xa0, 0x6c, 0x0c, 0x81, 0xba, 0xd8, 0x5b, 0x0c,
@@ -138,7 +149,7 @@ count_secrets(const char *buf)
return (res);
}
-static char *
+ATTRIBUTE_NO_SANITIZE_ADDRESS static char *
test_without_bzero(void)
{
char buf[SECRETBYTES];
@@ -149,7 +160,7 @@ test_without_bzero(void)
return (res);
}
-static char *
+ATTRIBUTE_NO_SANITIZE_ADDRESS static char *
test_with_bzero(void)
{
char buf[SECRETBYTES];
Re: explicit_bzero vs ASAN on linux
On Wed, Feb 09, 2022 at 09:09:35AM +0100, Theo Buehler wrote:
> In libressl-portable we run the explicit_bzero tests as part of the
> builds. If we enable ASAN on linux, this test segfaults in
> __interceptor_memmem() in the two test_with{,out}_bzero() functions,
> presumably because the sigaltstack magic is too low level for ASAN to
> grok.
>
> Would the patch below that disables ASAN for these functions be
> acceptable or shold we maintain it in the libressl-portable repo?
>
> https://github.com/google/sanitizers/wiki/AddressSanitizer#turning-off-instrumentation
>
> Index: explicit_bzero.c
> ===
> RCS file: /cvs/src/regress/lib/libc/explicit_bzero/explicit_bzero.c,v
> retrieving revision 1.8
> diff -u -p -r1.8 explicit_bzero.c
> --- explicit_bzero.c 9 Feb 2022 07:48:15 - 1.8
> +++ explicit_bzero.c 9 Feb 2022 07:56:43 -
> @@ -26,6 +26,12 @@
> #define ASSERT_NE(a, b) assert((a) != (b))
> #define ASSERT_GE(a, b) assert((a) >= (b))
>
> +#if defined(__clang__) || defined (__GNUC__)
> +#define ATTRIBUTE_NO_SANITIZE_ADDRESS __attribute__((no_sanitize_address))
> +#else
> +#define ATTRIBUTE_NO_SANITIZE_ADDRESS
> +#endif
> +
clang also defines __GNUC__, not sure if clang-cl is different
with clang 13.0.0 still 4.2.1
#define __GNUC__ 4
#define __GNUC_MINOR__ 2
#define __GNUC_PATCHLEVEL__ 1
I was curious how actual gcc 4.2.1 would handle this so I tried on
sparc64. The test still passes but there is a warning about an unknown
attribute.
run-regress-explicit_bzero
cc -O2 -pipe -MD -MP -c
/usr/src/regress/lib/libc/explicit_bzero/explicit_bzero.c
/usr/src/regress/lib/libc/explicit_bzero/explicit_bzero.c:149: warning:
'no_sanitize_address' attribute directive ignored
/usr/src/regress/lib/libc/explicit_bzero/explicit_bzero.c:160: warning:
'no_sanitize_address' attribute directive ignored
cc -o explicit_bzero explicit_bzero.o
./explicit_bzero
> /* 128 bits of random data. */
> static const char secret[16] = {
> 0xa0, 0x6c, 0x0c, 0x81, 0xba, 0xd8, 0x5b, 0x0c,
> @@ -138,7 +144,7 @@ count_secrets(const char *buf)
> return (res);
> }
>
> -static char *
> +ATTRIBUTE_NO_SANITIZE_ADDRESS static char *
> test_without_bzero(void)
> {
> char buf[SECRETBYTES];
> @@ -149,7 +155,7 @@ test_without_bzero(void)
> return (res);
> }
>
> -static char *
> +ATTRIBUTE_NO_SANITIZE_ADDRESS static char *
> test_with_bzero(void)
> {
> char buf[SECRETBYTES];
>
>
Re: wskbd_set_mixervolume
On Tue, Feb 08, 2022 at 06:59:39PM +0100, Anton Lindqvist wrote: > On Tue, Feb 08, 2022 at 07:32:38AM +0100, Alexandre Ratchov wrote: > > On Mon, Feb 07, 2022 at 06:55:21PM +0100, Anton Lindqvist wrote: > > > On Mon, Feb 07, 2022 at 11:21:43AM +0100, Alexandre Ratchov wrote: > > > > On Sun, Feb 06, 2022 at 08:40:35AM +0100, Anton Lindqvist wrote: > > > > > > > > > > Polished diff. I'm omitting a necessary refactoring commit making > > > > > audio_attach_mi() accept a new cookie argument. > > > > > > > > > > > > > I'm not sure to understand the need for the wskbd_audio structure. > > > > Couldn't we just store the cookie in audio and wskbd softc structures, > > > > then pass it in the wskbd_set_mixervolume_sc() calls ? > > > > > > Due to the device caching the data must be stored in either the audio or > > > wskbd softc and I don't want to expose the softc structures so I ended > > > up introducing wskbd_audio. Dropping the caching would probably make it > > > possible to only pass down the cookie to wskbd_set_mixervolume_sc() and > > > always do the audio device lookup. > > > > > > Is anyone in favor of this approach? I achieves the expected behavior in > > > my opinion. > > > > IMHO, handling the volume keys this way won't work in the general > > case. But for the short term we've no other options, have we? > > > > AFAICS, you're fixing a concrete use-case by tweaking what already > > exists, this won't make things more broken than they already are. I'm > > OK with it. > > Here's the complete diff including adding a cookie argument to > audio_attach_mi(). Is the caching necessary? device_lookup() seems cheap and there are at most two devices in most cases. Keeping the code minimal especially on rare and non-performace-critical code-paths would be nice. If you choose to drop the caching, this would allow to just add a a new "cookie" argument to wskbd_set_mixervolume(), similarly to what you did for audio_attach_mi()
explicit_bzero vs ASAN on linux
In libressl-portable we run the explicit_bzero tests as part of the
builds. If we enable ASAN on linux, this test segfaults in
__interceptor_memmem() in the two test_with{,out}_bzero() functions,
presumably because the sigaltstack magic is too low level for ASAN to
grok.
Would the patch below that disables ASAN for these functions be
acceptable or shold we maintain it in the libressl-portable repo?
https://github.com/google/sanitizers/wiki/AddressSanitizer#turning-off-instrumentation
Index: explicit_bzero.c
===
RCS file: /cvs/src/regress/lib/libc/explicit_bzero/explicit_bzero.c,v
retrieving revision 1.8
diff -u -p -r1.8 explicit_bzero.c
--- explicit_bzero.c9 Feb 2022 07:48:15 - 1.8
+++ explicit_bzero.c9 Feb 2022 07:56:43 -
@@ -26,6 +26,12 @@
#define ASSERT_NE(a, b) assert((a) != (b))
#define ASSERT_GE(a, b) assert((a) >= (b))
+#if defined(__clang__) || defined (__GNUC__)
+#define ATTRIBUTE_NO_SANITIZE_ADDRESS __attribute__((no_sanitize_address))
+#else
+#define ATTRIBUTE_NO_SANITIZE_ADDRESS
+#endif
+
/* 128 bits of random data. */
static const char secret[16] = {
0xa0, 0x6c, 0x0c, 0x81, 0xba, 0xd8, 0x5b, 0x0c,
@@ -138,7 +144,7 @@ count_secrets(const char *buf)
return (res);
}
-static char *
+ATTRIBUTE_NO_SANITIZE_ADDRESS static char *
test_without_bzero(void)
{
char buf[SECRETBYTES];
@@ -149,7 +155,7 @@ test_without_bzero(void)
return (res);
}
-static char *
+ATTRIBUTE_NO_SANITIZE_ADDRESS static char *
test_with_bzero(void)
{
char buf[SECRETBYTES];
