Re: [v3] amd64: simplify TSC sync testing
Sorry, my latest reply. I tested your patch on my Proxmox Virtual Environment on Ryzen7 box. It works fine for me. OpenBSD 7.1-current (GENERIC.MP) #1: Wed Jul 20 14:15:23 JST 2022 a...@pve-obsd.my.domain:/usr/src/sys/arch/amd64/compile/GENERIC.MP real mem = 17162952704 (16367MB) avail mem = 16625430528 (15855MB) random: good seed from bootblocks mpath0 at root scsibus0 at mpath0: 256 targets mainbus0 at root bios0 at mainbus0: SMBIOS rev. 2.8 @ 0xf59c0 (10 entries) bios0: vendor SeaBIOS version "rel-1.15.0-0-g2dd4b9b3f840-prebuilt.qemu.org" date 04/01/2014 bios0: QEMU Standard PC (i440FX + PIIX, 1996) acpi0 at bios0: ACPI 1.0 acpi0: sleep states S3 S4 S5 acpi0: tables DSDT FACP APIC SSDT HPET WAET acpi0: wakeup devices acpitimer0 at acpi0: 3579545 Hz, 24 bits acpimadt0 at acpi0 addr 0xfee0: PC-AT compat cpu0 at mainbus0: apid 0 (boot processor) cpu0: Common KVM processor, 3593.56 MHz, 0f-06-01 cpu0: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,CX16,x2APIC,HV,NXE,LONG,LAHF,CMPLEG cpu0: 64KB 64b/line 2-way D-cache, 64KB 64b/line 2-way I-cache cpu0: 512KB 64b/line 16-way L2 cache cpu0: smt 0, core 0, package 0 mtrr: Pentium Pro MTRR support, 8 var ranges, 88 fixed ranges cpu0: apic clock running at 999MHz cpu1 at mainbus0: apid 1 (application processor) cpu1: Common KVM processor, 3593.23 MHz, 0f-06-01 cpu1: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,CX16,x2APIC,HV,NXE,LONG,LAHF,CMPLEG cpu1: 64KB 64b/line 2-way D-cache, 64KB 64b/line 2-way I-cache cpu1: 512KB 64b/line 16-way L2 cache cpu1: smt 0, core 1, package 0 cpu2 at mainbus0: apid 2 (application processor) cpu2: Common KVM processor, 3593.22 MHz, 0f-06-01 cpu2: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,CX16,x2APIC,HV,NXE,LONG,LAHF,CMPLEG cpu2: 64KB 64b/line 2-way D-cache, 64KB 64b/line 2-way I-cache cpu2: 512KB 64b/line 16-way L2 cache cpu2: smt 0, core 2, package 0 cpu3 at mainbus0: apid 3 (application processor) cpu3: Common KVM processor, 3593.21 MHz, 0f-06-01 cpu3: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,CX16,x2APIC,HV,NXE,LONG,LAHF,CMPLEG cpu3: 64KB 64b/line 2-way D-cache, 64KB 64b/line 2-way I-cache cpu3: 512KB 64b/line 16-way L2 cache cpu3: smt 0, core 3, package 0 cpu4 at mainbus0: apid 4 (application processor) cpu4: Common KVM processor, 3593.23 MHz, 0f-06-01 cpu4: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,CX16,x2APIC,HV,NXE,LONG,LAHF,CMPLEG cpu4: 64KB 64b/line 2-way D-cache, 64KB 64b/line 2-way I-cache cpu4: 512KB 64b/line 16-way L2 cache cpu4: smt 0, core 4, package 0 cpu5 at mainbus0: apid 5 (application processor) cpu5: Common KVM processor, 3593.23 MHz, 0f-06-01 cpu5: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,CX16,x2APIC,HV,NXE,LONG,LAHF,CMPLEG cpu5: 64KB 64b/line 2-way D-cache, 64KB 64b/line 2-way I-cache cpu5: 512KB 64b/line 16-way L2 cache cpu5: smt 0, core 5, package 0 cpu6 at mainbus0: apid 6 (application processor) cpu6: Common KVM processor, 3593.22 MHz, 0f-06-01 cpu6: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,CX16,x2APIC,HV,NXE,LONG,LAHF,CMPLEG cpu6: 64KB 64b/line 2-way D-cache, 64KB 64b/line 2-way I-cache cpu6: 512KB 64b/line 16-way L2 cache cpu6: smt 0, core 6, package 0 cpu7 at mainbus0: apid 7 (application processor) cpu7: Common KVM processor, 3593.23 MHz, 0f-06-01 cpu7: FPU,VME,DE,PSE,TSC,MSR,PAE,MCE,CX8,APIC,SEP,MTRR,PGE,MCA,CMOV,PAT,PSE36,CFLUSH,MMX,FXSR,SSE,SSE2,HTT,SSE3,CX16,x2APIC,HV,NXE,LONG,LAHF,CMPLEG cpu7: 64KB 64b/line 2-way D-cache, 64KB 64b/line 2-way I-cache cpu7: 512KB 64b/line 16-way L2 cache cpu7: smt 0, core 7, package 0 ioapic0 at mainbus0: apid 0 pa 0xfec0, version 11, 24 pins acpihpet0 at acpi0: 1 Hz acpiprt0 at acpi0: bus 0 (PCI0) "ACPI0006" at acpi0 not configured acpipci0 at acpi0 PCI0 acpicmos0 at acpi0 "PNP0A06" at acpi0 not configured "PNP0A06" at acpi0 not configured "PNP0A06" at acpi0 not configured "QEMU0002" at acpi0 not configured "ACPI0010" at acpi0 not configured "QEMUVGID" at acpi0 not configured acpicpu0 at acpi0: C1(@1 halt!) acpicpu1 at acpi0: C1(@1 halt!) acpicpu2 at acpi0: C1(@1 halt!) acpicpu3 at acpi0: C1(@1 halt!) acpicpu4 at acpi0: C1(@1 halt!) acpicpu5 at acpi0: C1(@1 halt!) acpicpu6 at acpi0: C1(@1 halt!) acpicpu7 at acpi0: C1(@1 halt!) pvbus0 at mainbus0: KVM pvclock0 at pvbus0 pci0 at mainbus0 bus 0 pchb0 at pci0 dev 0 function 0 "Intel 82441FX" rev 0x02 pcib0 at pci0 dev 1 function 0 "Intel 82371SB ISA" rev 0x00 pciide0 at pci0 dev 1 function 1 "Intel 82371SB IDE" rev 0x00: DMA, channel 0 wired to compatibility, channel 1 wired to compatibility wd0 at pciide0 channel 0 drive 0: wd0: 16-sector PIO, LBA48, 65
Re: pf: pool for pf_anchor
On Tue, Jul 19, 2022 at 07:18:54PM +0200, Moritz Buhl wrote:
> A solution would be to move the allocation of the pf_anchor struct
> into a pool. One final question would be regarding the size of the
> hiwat or hardlimit. Any suggestions?
10 seems way to low. We want to limit resource abuse. But regular
users should never hit this. Especially existing configurations
should still work after upgrade.
Perhaps 512 anchors. Noone shoul ever need more than 512 anchors :-)
> Any other comments?
Please do not use PR_WAITOK in pool_init() unless you know what you
are doing. I think it should work, but who knows. The other pf
pools have 0 flags, just use that.
Comment says rs_malloc is needed to compile pfctl. Have you tested
that?
Replacing M_WAITOK|M_CANFAIL|M_ZERO with PR_NOWAIT|PR_ZERO looks
wrong. PR_WAITOK|PR_LIMITFAIL|PR_ZERO should do something equivalent.
I am not sure if pf_find_anchor() should fail if the limit is hit.
It uses only a temporary buffer. All calls to pf_find_ruleset()
should be checked whether permanent failure, after the limit has
been reached, is what we want. Or we keep the malloc there?
bluhm
> Index: sys/net/pf.c
> ===
> RCS file: /mount/openbsd/cvs/src/sys/net/pf.c,v
> retrieving revision 1.1135
> diff -u -p -r1.1135 pf.c
> --- sys/net/pf.c 28 Jun 2022 13:48:06 - 1.1135
> +++ sys/net/pf.c 19 Jul 2022 11:34:23 -
> @@ -269,6 +269,7 @@ void pf_log_matches(struct pf_pdesc
> *
>
> extern struct pool pfr_ktable_pl;
> extern struct pool pfr_kentry_pl;
> +extern struct pool pf_anchor_pl;
>
> struct pf_pool_limit pf_pool_limits[PF_LIMIT_MAX] = {
> { &pf_state_pl, PFSTATE_HIWAT, PFSTATE_HIWAT },
> @@ -276,7 +277,8 @@ struct pf_pool_limit pf_pool_limits[PF_L
> { &pf_frent_pl, PFFRAG_FRENT_HIWAT, PFFRAG_FRENT_HIWAT },
> { &pfr_ktable_pl, PFR_KTABLE_HIWAT, PFR_KTABLE_HIWAT },
> { &pfr_kentry_pl, PFR_KENTRY_HIWAT, PFR_KENTRY_HIWAT },
> - { &pf_pktdelay_pl, PF_PKTDELAY_MAXPKTS, PF_PKTDELAY_MAXPKTS }
> + { &pf_pktdelay_pl, PF_PKTDELAY_MAXPKTS, PF_PKTDELAY_MAXPKTS },
> + { &pf_anchor_pl, PF_ANCHOR_HIWAT, PF_ANCHOR_HIWAT }
> };
>
> #define BOUND_IFACE(r, k) \
> Index: sys/net/pf_ioctl.c
> ===
> RCS file: /mount/openbsd/cvs/src/sys/net/pf_ioctl.c,v
> retrieving revision 1.382
> diff -u -p -r1.382 pf_ioctl.c
> --- sys/net/pf_ioctl.c26 Jun 2022 11:37:08 - 1.382
> +++ sys/net/pf_ioctl.c19 Jul 2022 16:56:23 -
> @@ -191,6 +191,8 @@ pfattach(int num)
> IPL_SOFTNET, 0, "pftag", NULL);
> pool_init(&pf_pktdelay_pl, sizeof(struct pf_pktdelay), 0,
> IPL_SOFTNET, 0, "pfpktdelay", NULL);
> + pool_init(&pf_anchor_pl, sizeof(struct pf_anchor), 0,
> + IPL_SOFTNET, PR_WAITOK, "pfanchor", NULL);
>
> hfsc_initialize();
> pfr_initialize();
> @@ -200,6 +202,8 @@ pfattach(int num)
>
> pool_sethardlimit(pf_pool_limits[PF_LIMIT_STATES].pp,
> pf_pool_limits[PF_LIMIT_STATES].limit, NULL, 0);
> + pool_sethardlimit(pf_pool_limits[PF_LIMIT_ANCHORS].pp,
> + pf_pool_limits[PF_LIMIT_ANCHORS].limit, NULL, 0);
>
> if (physmem <= atop(100*1024*1024))
> pf_pool_limits[PF_LIMIT_TABLE_ENTRIES].limit =
> Index: sys/net/pf_ruleset.c
> ===
> RCS file: /mount/openbsd/cvs/src/sys/net/pf_ruleset.c,v
> retrieving revision 1.18
> diff -u -p -r1.18 pf_ruleset.c
> --- sys/net/pf_ruleset.c 27 Dec 2018 16:54:01 - 1.18
> +++ sys/net/pf_ruleset.c 19 Jul 2022 12:56:35 -
> @@ -40,6 +40,7 @@
> #ifdef _KERNEL
> #include
> #include
> +#include
> #endif /* _KERNEL */
> #include
>
> @@ -55,6 +56,7 @@
> #endif /* INET6 */
>
>
> +struct pool pf_anchor_pl;
> #ifdef _KERNEL
> #define rs_malloc(x) malloc(x, M_TEMP, M_WAITOK|M_CANFAIL|M_ZERO)
> #define rs_free(x, siz) free(x, M_TEMP, siz)
> @@ -107,12 +109,12 @@ pf_find_anchor(const char *path)
> {
> struct pf_anchor*key, *found;
>
> - key = rs_malloc(sizeof(*key));
> + key = pool_get(&pf_anchor_pl, PR_NOWAIT | PR_ZERO);
> if (key == NULL)
> return (NULL);
> strlcpy(key->path, path, sizeof(key->path));
> found = RB_FIND(pf_anchor_global, &pf_anchors, key);
> - rs_free(key, sizeof(*key));
> + pool_put(&pf_anchor_pl, key);
> return (found);
> }
>
> @@ -184,7 +186,7 @@ pf_create_anchor(struct pf_anchor *paren
> ((parent != NULL) && (strlen(parent->path) >= PF_ANCHOR_MAXPATH)))
> return (NULL);
>
> - anchor = rs_malloc(sizeof(*anchor));
> + anchor = pool_get(&pf_anchor_pl, PR_NOWAIT | PR_ZERO);
> if (anchor == NULL)
> return (NULL);
>
> @@ -204,7 +206,7 @@ pf_create_anchor(struct pf_anchor *paren
>
pf: pool for pf_anchor
Dear tech@,
I am investigating a syzkaller reproducer found in the
"no output from test machine (7)" crashes:
https://syzkaller.appspot.com/bug?id=d93e92fde3857c69df2cf46b4244d9814c4318a7
https://syzkaller.appspot.com/text?tag=ReproC&x=116ee2e008
The code calls DIOCXBEGIN, with different anchor names
until the malloc bucket for size 2048 fills up.
So the following reproducer does the same:
#include
#include
#include
#include
#include
#include
#include
int calls;
void
iterate(char *str)
{
char *end = str + 64;
*end = '\0';
while (*str == '\255') {
*str = '\1';
str++;
}
if (str == end) return;
if (*str == '\0') *str = '\1';
*str = ++*str;
}
int
main(void)
{
struct pfioc_trans trans;
struct pfioc_trans_e elem;
int fd, ret;
char it[64] = "\0";
fd = open("/dev/pf", O_RDWR);
while (1) {
trans.size = 1;
trans.esize = 0x408;
trans.array = &elem;
elem.type = 0;
iterate(it);
strcpy(elem.anchor, it);
elem.ticket = 0;
calls++;
if (ioctl(fd, DIOCXBEGIN, &trans) == -1)
err(1, "icoctl: %d calls, '%s'", calls, elem.anchor);
}
}
A solution would be to move the allocation of the pf_anchor struct
into a pool. One final question would be regarding the size of the
hiwat or hardlimit. Any suggestions?
Any other comments?
mbuhl
Index: sys/net/pf.c
===
RCS file: /mount/openbsd/cvs/src/sys/net/pf.c,v
retrieving revision 1.1135
diff -u -p -r1.1135 pf.c
--- sys/net/pf.c28 Jun 2022 13:48:06 - 1.1135
+++ sys/net/pf.c19 Jul 2022 11:34:23 -
@@ -269,6 +269,7 @@ void pf_log_matches(struct pf_pdesc
*
extern struct pool pfr_ktable_pl;
extern struct pool pfr_kentry_pl;
+extern struct pool pf_anchor_pl;
struct pf_pool_limit pf_pool_limits[PF_LIMIT_MAX] = {
{ &pf_state_pl, PFSTATE_HIWAT, PFSTATE_HIWAT },
@@ -276,7 +277,8 @@ struct pf_pool_limit pf_pool_limits[PF_L
{ &pf_frent_pl, PFFRAG_FRENT_HIWAT, PFFRAG_FRENT_HIWAT },
{ &pfr_ktable_pl, PFR_KTABLE_HIWAT, PFR_KTABLE_HIWAT },
{ &pfr_kentry_pl, PFR_KENTRY_HIWAT, PFR_KENTRY_HIWAT },
- { &pf_pktdelay_pl, PF_PKTDELAY_MAXPKTS, PF_PKTDELAY_MAXPKTS }
+ { &pf_pktdelay_pl, PF_PKTDELAY_MAXPKTS, PF_PKTDELAY_MAXPKTS },
+ { &pf_anchor_pl, PF_ANCHOR_HIWAT, PF_ANCHOR_HIWAT }
};
#define BOUND_IFACE(r, k) \
Index: sys/net/pf_ioctl.c
===
RCS file: /mount/openbsd/cvs/src/sys/net/pf_ioctl.c,v
retrieving revision 1.382
diff -u -p -r1.382 pf_ioctl.c
--- sys/net/pf_ioctl.c 26 Jun 2022 11:37:08 - 1.382
+++ sys/net/pf_ioctl.c 19 Jul 2022 16:56:23 -
@@ -191,6 +191,8 @@ pfattach(int num)
IPL_SOFTNET, 0, "pftag", NULL);
pool_init(&pf_pktdelay_pl, sizeof(struct pf_pktdelay), 0,
IPL_SOFTNET, 0, "pfpktdelay", NULL);
+ pool_init(&pf_anchor_pl, sizeof(struct pf_anchor), 0,
+ IPL_SOFTNET, PR_WAITOK, "pfanchor", NULL);
hfsc_initialize();
pfr_initialize();
@@ -200,6 +202,8 @@ pfattach(int num)
pool_sethardlimit(pf_pool_limits[PF_LIMIT_STATES].pp,
pf_pool_limits[PF_LIMIT_STATES].limit, NULL, 0);
+ pool_sethardlimit(pf_pool_limits[PF_LIMIT_ANCHORS].pp,
+ pf_pool_limits[PF_LIMIT_ANCHORS].limit, NULL, 0);
if (physmem <= atop(100*1024*1024))
pf_pool_limits[PF_LIMIT_TABLE_ENTRIES].limit =
Index: sys/net/pf_ruleset.c
===
RCS file: /mount/openbsd/cvs/src/sys/net/pf_ruleset.c,v
retrieving revision 1.18
diff -u -p -r1.18 pf_ruleset.c
--- sys/net/pf_ruleset.c27 Dec 2018 16:54:01 - 1.18
+++ sys/net/pf_ruleset.c19 Jul 2022 12:56:35 -
@@ -40,6 +40,7 @@
#ifdef _KERNEL
#include
#include
+#include
#endif /* _KERNEL */
#include
@@ -55,6 +56,7 @@
#endif /* INET6 */
+struct poolpf_anchor_pl;
#ifdef _KERNEL
#define rs_malloc(x) malloc(x, M_TEMP, M_WAITOK|M_CANFAIL|M_ZERO)
#define rs_free(x, siz)free(x, M_TEMP, siz)
@@ -107,12 +109,12 @@ pf_find_anchor(const char *path)
{
struct pf_anchor*key, *found;
- key = rs_malloc(sizeof(*key));
+ key = pool_get(&pf_anchor_pl, PR_NOWAIT | PR_ZERO);
if (key == NULL)
return (NULL);
strlcpy(key->path, path, sizeof(key->path));
found = RB_FIND(pf_anchor_global, &pf_anchors, key);
- rs_free(key, sizeof(*key));
+ pool_put(&pf_anchor_pl, key);
return (found);
}
@@ -184,7 +186,7 @@ pf_create_anchor(struct pf_anchor *paren
((parent != NULL) && (strlen(parent->path
Re: bgpd aspath_extract overflow check
On Tue, Jul 19, 2022 at 12:31:47PM +0200, Theo Buehler wrote: > On Tue, Jul 19, 2022 at 11:43:25AM +0200, Claudio Jeker wrote: > > aspath_extract() should do at least a minimal overflow check and not > > access memory after the segment. Can't use fatalx here because bgpctl > > also uses this function. Instead return 0, that is an invalid ASN. > > No code will check the return value but that is fine since all callers > > ensure that pos does not overflow. > > There are a few calls of the form > > as = aspath_extract(seg, seg_len - 1) > > where seg_len = ptr[1], so a priori pos == -1 seems possible. Should the > check not be > > if (pos < 0 || pos >= ptr[1]) > return 0; > > to exclude an access at ptr[-2]? Makes sense. It is again not really an issue because aspath_verify() filters those ASPATHs out and aspath_extract() should only be called on valid ASPATHs. I will commit this with the pos < 0 check. > > > > -- > > :wq Claudio > > > > Index: util.c > > === > > RCS file: /cvs/src/usr.sbin/bgpd/util.c,v > > retrieving revision 1.69 > > diff -u -p -r1.69 util.c > > --- util.c 28 Jun 2022 05:49:05 - 1.69 > > +++ util.c 28 Jun 2022 08:31:10 - > > @@ -364,7 +364,7 @@ aspath_strlen(void *data, uint16_t len) > > /* > > * Extract the asnum out of the as segment at the specified position. > > * Direct access is not possible because of non-aligned reads. > > - * ATTENTION: no bounds checks are done. > > + * Only works on verified 4-byte AS paths. > > */ > > uint32_t > > aspath_extract(const void *seg, int pos) > > @@ -372,6 +372,9 @@ aspath_extract(const void *seg, int pos) > > const u_char*ptr = seg; > > uint32_t as; > > > > + /* minimal overflow check, return 0 since that is an invalid ASN */ > > + if (pos >= ptr[1]) > > + return (0); > > ptr += 2 + sizeof(uint32_t) * pos; > > memcpy(&as, ptr, sizeof(uint32_t)); > > return (ntohl(as)); > > > -- :wq Claudio
Re: bgpd aspath_extract overflow check
On Tue, Jul 19, 2022 at 11:43:25AM +0200, Claudio Jeker wrote: > aspath_extract() should do at least a minimal overflow check and not > access memory after the segment. Can't use fatalx here because bgpctl > also uses this function. Instead return 0, that is an invalid ASN. > No code will check the return value but that is fine since all callers > ensure that pos does not overflow. There are a few calls of the form as = aspath_extract(seg, seg_len - 1) where seg_len = ptr[1], so a priori pos == -1 seems possible. Should the check not be if (pos < 0 || pos >= ptr[1]) return 0; to exclude an access at ptr[-2]? > > -- > :wq Claudio > > Index: util.c > === > RCS file: /cvs/src/usr.sbin/bgpd/util.c,v > retrieving revision 1.69 > diff -u -p -r1.69 util.c > --- util.c28 Jun 2022 05:49:05 - 1.69 > +++ util.c28 Jun 2022 08:31:10 - > @@ -364,7 +364,7 @@ aspath_strlen(void *data, uint16_t len) > /* > * Extract the asnum out of the as segment at the specified position. > * Direct access is not possible because of non-aligned reads. > - * ATTENTION: no bounds checks are done. > + * Only works on verified 4-byte AS paths. > */ > uint32_t > aspath_extract(const void *seg, int pos) > @@ -372,6 +372,9 @@ aspath_extract(const void *seg, int pos) > const u_char*ptr = seg; > uint32_t as; > > + /* minimal overflow check, return 0 since that is an invalid ASN */ > + if (pos >= ptr[1]) > + return (0); > ptr += 2 + sizeof(uint32_t) * pos; > memcpy(&as, ptr, sizeof(uint32_t)); > return (ntohl(as)); >
Re: bgpd name struct kroute_full vars kf
On Tue, Jul 19, 2022 at 12:08:40PM +0200, Claudio Jeker wrote: > Use kf for all struct kroute_full variables in bgpd. This makes the code > more consistent. ok
bgpd name struct kroute_full vars kf
Use kf for all struct kroute_full variables in bgpd. This makes the code
more consistent.
--
:wq Claudio
Index: kroute.c
===
RCS file: /cvs/src/usr.sbin/bgpd/kroute.c,v
retrieving revision 1.273
diff -u -p -r1.273 kroute.c
--- kroute.c18 Jul 2022 23:09:44 - 1.273
+++ kroute.c19 Jul 2022 10:05:51 -
@@ -471,22 +471,22 @@ ktable_exists(u_int rtableid, u_int *rdo
}
int
-kr_change(u_int rtableid, struct kroute_full *kl)
+kr_change(u_int rtableid, struct kroute_full *kf)
{
struct ktable *kt;
if ((kt = ktable_get(rtableid)) == NULL)
/* too noisy during reloads, just ignore */
return (0);
- switch (kl->prefix.aid) {
+ switch (kf->prefix.aid) {
case AID_INET:
- return (kr4_change(kt, kl));
+ return (kr4_change(kt, kf));
case AID_INET6:
- return (kr6_change(kt, kl));
+ return (kr6_change(kt, kf));
case AID_VPN_IPv4:
- return (krVPN4_change(kt, kl));
+ return (krVPN4_change(kt, kf));
case AID_VPN_IPv6:
- return (krVPN6_change(kt, kl));
+ return (krVPN6_change(kt, kf));
}
log_warnx("%s: not handled AID", __func__);
return (-1);
@@ -764,7 +764,7 @@ krVPN6_change(struct ktable *kt, struct
}
int
-kr_delete(u_int rtableid, struct kroute_full *kl)
+kr_delete(u_int rtableid, struct kroute_full *kf)
{
struct ktable *kt;
@@ -772,15 +772,15 @@ kr_delete(u_int rtableid, struct kroute_
/* too noisy during reloads, just ignore */
return (0);
- switch (kl->prefix.aid) {
+ switch (kf->prefix.aid) {
case AID_INET:
- return (kr4_delete(kt, kl));
+ return (kr4_delete(kt, kf));
case AID_INET6:
- return (kr6_delete(kt, kl));
+ return (kr6_delete(kt, kf));
case AID_VPN_IPv4:
- return (krVPN4_delete(kt, kl));
+ return (krVPN4_delete(kt, kf));
case AID_VPN_IPv6:
- return (krVPN6_delete(kt, kl));
+ return (krVPN6_delete(kt, kf));
}
log_warnx("%s: not handled AID", __func__);
return (-1);
@@ -819,11 +819,11 @@ kr_flush(u_int rtableid)
}
int
-kr4_delete(struct ktable *kt, struct kroute_full *kl)
+kr4_delete(struct ktable *kt, struct kroute_full *kf)
{
struct kroute *kr;
- if ((kr = kroute_find(kt, &kl->prefix, kl->prefixlen,
+ if ((kr = kroute_find(kt, &kf->prefix, kf->prefixlen,
RTP_MINE)) == NULL)
return (0);
@@ -839,11 +839,11 @@ kr4_delete(struct ktable *kt, struct kro
}
int
-kr6_delete(struct ktable *kt, struct kroute_full *kl)
+kr6_delete(struct ktable *kt, struct kroute_full *kf)
{
struct kroute6 *kr6;
- if ((kr6 = kroute6_find(kt, &kl->prefix, kl->prefixlen,
+ if ((kr6 = kroute6_find(kt, &kf->prefix, kf->prefixlen,
RTP_MINE)) == NULL)
return (0);
@@ -859,11 +859,11 @@ kr6_delete(struct ktable *kt, struct kro
}
int
-krVPN4_delete(struct ktable *kt, struct kroute_full *kl)
+krVPN4_delete(struct ktable *kt, struct kroute_full *kf)
{
struct kroute *kr;
- if ((kr = kroute_find(kt, &kl->prefix, kl->prefixlen,
+ if ((kr = kroute_find(kt, &kf->prefix, kf->prefixlen,
RTP_MINE)) == NULL)
return (0);
@@ -879,11 +879,11 @@ krVPN4_delete(struct ktable *kt, struct
}
int
-krVPN6_delete(struct ktable *kt, struct kroute_full *kl)
+krVPN6_delete(struct ktable *kt, struct kroute_full *kf)
{
struct kroute6 *kr6;
- if ((kr6 = kroute6_find(kt, &kl->prefix, kl->prefixlen,
+ if ((kr6 = kroute6_find(kt, &kf->prefix, kf->prefixlen,
RTP_MINE)) == NULL)
return (0);
@@ -3042,7 +3042,7 @@ fetchtable(struct ktable *kt)
int mib[7];
char*buf = NULL, *next, *lim;
struct rt_msghdr*rtm;
- struct kroute_full kl;
+ struct kroute_full kf;
struct kroute *kr;
struct kroute6 *kr6;
@@ -3079,49 +3079,49 @@ fetchtable(struct ktable *kt)
if (rtm->rtm_version != RTM_VERSION)
continue;
- if (dispatch_rtmsg_addr(rtm, &kl) == -1)
+ if (dispatch_rtmsg_addr(rtm, &kf) == -1)
continue;
- if (kl.prefix.aid == AID_INET) {
+ if (kf.prefix.aid == AID_INET) {
if ((kr = calloc(1, sizeof(*kr))) == NULL) {
log_warn("%s", __func__);
return (-1);
}
- kr->prefix.s_addr = kl.prefix.v4.s_addr;
- kr->pr
bgpd aspath_extract overflow check
aspath_extract() should do at least a minimal overflow check and not access memory after the segment. Can't use fatalx here because bgpctl also uses this function. Instead return 0, that is an invalid ASN. No code will check the return value but that is fine since all callers ensure that pos does not overflow. -- :wq Claudio Index: util.c === RCS file: /cvs/src/usr.sbin/bgpd/util.c,v retrieving revision 1.69 diff -u -p -r1.69 util.c --- util.c 28 Jun 2022 05:49:05 - 1.69 +++ util.c 28 Jun 2022 08:31:10 - @@ -364,7 +364,7 @@ aspath_strlen(void *data, uint16_t len) /* * Extract the asnum out of the as segment at the specified position. * Direct access is not possible because of non-aligned reads. - * ATTENTION: no bounds checks are done. + * Only works on verified 4-byte AS paths. */ uint32_t aspath_extract(const void *seg, int pos) @@ -372,6 +372,9 @@ aspath_extract(const void *seg, int pos) const u_char*ptr = seg; uint32_t as; + /* minimal overflow check, return 0 since that is an invalid ASN */ + if (pos >= ptr[1]) + return (0); ptr += 2 + sizeof(uint32_t) * pos; memcpy(&as, ptr, sizeof(uint32_t)); return (ntohl(as));
