On Dec 29, 10:09pm, alnsn%yandex.ru@localhost (Alexander Nasonov) wrote:
-- Subject: Re: CVS commit: src/sys/net
| Christos Zoulas wrote:
| > Module Name: src
| > Committed By: christos
| > Date: Thu Dec 29 20:50:06 UTC 2011
| >
| > Modified Files:
| > src/sys/net: bpf_filter.c
| >
| > Log Message:
| > PR/45751: Alexander Nasonov: No overflow check in BPF_LD|BPF_ABS
| >
| >
| > To generate a diff of this commit:
| > cvs rdiff -u -r1.48 -r1.49 src/sys/net/bpf_filter.c
| ...
| > @@ -253,7 +254,8 @@ bpf_filter(const struct bpf_insn *pc, co
| >
| > case BPF_LD|BPF_H|BPF_IND:
| > k = X + pc->k;
| > - if (k + sizeof(int16_t) > buflen) {
| > + if (pc->k > buflen || X > buflen - pc->k ||
| > + sizeof(int16_t) > buflen - k) {
| > #ifdef _KERNEL
| > int merr = 0; /* XXX: GCC */
|
| Not sure FreeBSD got BPF_IND case right. They basically disabled using
| big positive values of pc->k for small negative values. They could just
| copy code from BPF_ABS case:
|
| > + if (k > buflen || sizeof(int16_t) > buflen - k) {
|
| but they didn't. Can we assume that loads with negative offsets relative
| to X (e.g. P[X-1:4]) are not allowed by bpf?
I suppose by turning k unsigned, they really want to disable negative offsets.
We could allow them if needed, but at that point it is better to make k signed.
christos