Skip to content

Prefer NPCAP_AD prefix for Npcap over SKF_AD prefix #1478

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Prefer NPCAP_AD prefix for Npcap over SKF_AD prefix #1478

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ab883cc
Prefer NPCAP_AD prefix for Npcap over SKF_AD prefix
bonsaiviking Feb 26, 2025
44fcd2c
Fix typo: parentheses after #ifdef
dmiller-nmap Feb 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions bpf_filter.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ enum {
*
* Thanks to Ani Sinha <[email protected]> for providing initial implementation
*/
#if defined(SKF_AD_VLAN_TAG_PRESENT)
/* Npcap does not provide auxiliary data */
#if defined(SKF_AD_VLAN_TAG_PRESENT) && !defined(NPCAP_AD_VLAN_TAG_PRESENT)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it true in this context that so long as SKF_AD_VLAN_TAG_PRESENT is defined, NPCAP_AD_VLAN_TAG_PRESENT is never defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As it currently stands, Npcap's npcap-bpf.h conditionally defines the same SKF_AD_* constants it did before (https://github.com/nmap/npcap/blob/a4e1690925da1c0086400a1dab7688567818ec5f/Common/npcap-bpf.h#L230-L238):

/* Npcap SDK 1.15 defined these names instead, so they are preserved here for
 * compatibility and to simplify integration with existing Linux-targeted code.
 * However, it is preferred to use the NPCAP_AD_* prefix in new code. */
#ifndef SKF_AD_OFF
#define SKF_AD_OFF              NPCAP_AD_OFF
#define SKF_AD_VLAN_TAG         NPCAP_AD_VLAN_TAG
#define SKF_AD_VLAN_TAG_PRESENT NPCAP_AD_VLAN_TAG_PRESENT
#define SKF_AD_MAX              NPCAP_AD_MAX
#endif

So in a Npcap build, both constants are defined. We can still change this if necessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on what you write, as far as I see, if there is a good way to remove SKF_AD_VLAN_TAG_PRESENT from Npcap scope now, it would be best to do that, then this specific change will not be required and things will be a bit simpler; if it is too late for that, this comment needs to explain this context a bit better:

/*
 * Npcap does not provide auxiliary data.  However, it can define
 * SKF_AD_VLAN_TAG_PRESENT if it defines NPCAP_AD_VLAN_TAG_PRESENT.
 */

u_int
pcapint_filter_with_aux_data(const struct bpf_insn *pc, const u_char *p,
u_int wirelen, u_int buflen, const struct pcap_bpf_aux_data *aux_data)
Expand Down Expand Up @@ -142,7 +143,7 @@ pcapint_filter_with_aux_data(const struct bpf_insn *pc, const u_char *p,
DIAG_OFF_DEFAULT_ONLY_SWITCH
switch (pc->k) {

#if defined(SKF_AD_VLAN_TAG_PRESENT)
#if defined(SKF_AD_VLAN_TAG_PRESENT) && !defined(NPCAP_AD_VLAN_TAG_PRESENT)
case SKF_AD_OFF + SKF_AD_VLAN_TAG:
if (!aux_data)
return 0;
Expand Down
29 changes: 20 additions & 9 deletions bpf_image.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,18 @@
#include "os-proto.h"
#endif

#ifdef SKF_AD_OFF
#if defined(SKF_AD_OFF) || defined(NPCAP_AD_OFF)
/*
* Symbolic names for offsets that refer to the special Linux BPF locations.
* Symbolic names for special offsets that refer to platform-specific BPF extensions.
*/
static const char *offsets[SKF_AD_MAX] = {
#ifdef NPCAP_AD_MAX
#define BPFEXT_AD_OFF NPCAP_AD_OFF
#define BPFEXT_AD_MAX NPCAP_AD_MAX
#else
#define BPFEXT_AD_OFF SKF_AD_OFF
#define BPFEXT_AD_MAX SKF_AD_MAX
#endif
static const char *offsets[BPFEXT_AD_MAX] = {
#ifdef SKF_AD_PROTOCOL
[SKF_AD_PROTOCOL] = "proto",
#endif
Expand Down Expand Up @@ -86,10 +93,14 @@ static const char *offsets[SKF_AD_MAX] = {
#ifdef SKF_AD_ALU_XOR_X
[SKF_AD_ALU_XOR_X] = "xor_x",
#endif
#ifdef SKF_AD_VLAN_TAG
#ifdef NPCAP_AD_VLAN_TAG
[NPCAP_AD_VLAN_TAG] = "vlan_tci",
#elif defined(SKF_AD_VLAN_TAG)
[SKF_AD_VLAN_TAG] = "vlan_tci",
#endif
#ifdef SKF_AD_VLAN_TAG_PRESENT
#ifdef NPCAP_AD_VLAN_TAG_PRESENT
[NPCAP_AD_VLAN_TAG_PRESENT] = "vlanp",
#elif defined(SKF_AD_VLAN_TAG_PRESENT)
[SKF_AD_VLAN_TAG_PRESENT] = "vlanp",
#endif
#ifdef SKF_AD_PAY_OFFSET
Expand All @@ -107,16 +118,16 @@ static const char *offsets[SKF_AD_MAX] = {
static void
bpf_print_abs_load_operand(char *buf, size_t bufsize, const struct bpf_insn *p)
{
#ifdef SKF_AD_OFF
#ifdef BPFEXT_AD_OFF
const char *sym;

/*
* It's an absolute load.
* Is the offset a special Linux offset that we know about?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this block is no longer Linux-specific, the comment needs to mention either both possibilities, or none.

*/
if (p->k >= (bpf_u_int32)SKF_AD_OFF &&
p->k < (bpf_u_int32)(SKF_AD_OFF + SKF_AD_MAX) &&
(sym = offsets[p->k - (bpf_u_int32)SKF_AD_OFF]) != NULL) {
if (p->k >= (bpf_u_int32)BPFEXT_AD_OFF &&
p->k < (bpf_u_int32)(BPFEXT_AD_OFF + BPFEXT_AD_MAX) &&
(sym = offsets[p->k - (bpf_u_int32)BPFEXT_AD_OFF]) != NULL) {
/*
* Yes. Print the offset symbolically.
*/
Expand Down
9 changes: 8 additions & 1 deletion gencode.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9222,6 +9222,13 @@ gen_vlan_no_bpf_extensions(compiler_state_t *cstate, bpf_u_int32 vlan_num,
return b0;
}

/* Npcap defines these constants in a way that is compatible with Linux, as
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"...compatible with Linux at source code level"

* long as no assumptions are made about the value or ordering. */
#if defined(NPCAP_AD_OFF) && !defined(SKF_AD_OFF)
#define SKF_AD_OFF NPCAP_AD_OFF
#define SKF_AD_VLAN_TAG NPCAP_AD_VLAN_TAG
#define SKF_AD_VLAN_TAG_PRESENT NPCAP_AD_VLAN_TAG_PRESENT
#endif
#if defined(SKF_AD_VLAN_TAG_PRESENT)
/* add v to variable part of off */
static void
Expand Down Expand Up @@ -9418,7 +9425,7 @@ gen_vlan(compiler_state_t *cstate, bpf_u_int32 vlan_num, int has_vlan_tag)
*
* This requires special treatment.
*/
#if defined(SKF_AD_VLAN_TAG_PRESENT)
#if defined(SKF_AD_VLAN_TAG_PRESENT) || defined(NPCAP_AD_VLAN_TAG_PRESENT)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it true in this context that so long as SKF_AD_VLAN_TAG_PRESENT is defined, it does not matter if NPCAP_AD_VLAN_TAG_PRESENT is defined, and so long as SKF_AD_VLAN_TAG_PRESENT is not defined, NPCAP_AD_VLAN_TAG_PRESENT is certainly not defined? In other words, changing this line seems to be a no-op.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I intended this to draw attention to the fact that this block is active in a Npcap build as well, clarifying intent of the preprocessor directive, especially in contrast to other places in this file where SKF_AD_* constants are instead guarded with #if defined(__linux__).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this context it seems much easier to follow if this specific change was just:

--- a/gencode.c
+++ b/gencode.c
@@ -8871,6 +8871,12 @@ gen_vlan(compiler_state_t *cstate, bpf_u_int32 vlan_num, int has_vlan_tag)
                 * Newer version of the Linux kernel pass around
                 * packets in which the VLAN tag has been removed
                 * from the packet data and put into metadata.
+                * (Specifically, the kernel started defining the constant
+                * in release 3.8.13 and started using it for packet
+                * filtering in release 3.15.8.)
+                *
+                * Npcap since version 1.81 supports the same feature, in
+                * which case the same constant has been defined by now.
                 *
                 * This requires special treatment.
                 */

/* Verify that this is the outer part of the packet and
* not encapsulated somehow. */
if (cstate->vlan_stack_depth == 0 && !cstate->off_linkhdr.is_variable &&
Expand Down
9 changes: 7 additions & 2 deletions pcap-npf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1090,13 +1090,18 @@ pcap_activate_npf(pcap_t *p)
}
#endif /* HAVE_PACKET_GET_TIMESTAMP_MODES */

#if defined(HAVE_PACKET_GET_INFO) && defined(NPF_GETINFO_BPFEXT) && defined(SKF_AD_VLAN_TAG_PRESENT)
/* Npcap SDK 1.15 defined SKF_AD_* constants; later versions prefer NPCAP_AD_* */
#if !defined(NPCAP_AD_VLAN_TAG_PRESENT) && defined(SKF_AD_VLAN_TAG_PRESENT)
#define NPCAP_AD_VLAN_TAG_PRESENT SKF_AD_VLAN_TAG_PRESENT
#endif

#if defined(HAVE_PACKET_GET_INFO) && defined(NPF_GETINFO_BPFEXT) && defined(NPCAP_AD_VLAN_TAG_PRESENT)

/* Can we generate special code for VLAN checks? */
oid_data_arg->Oid = NPF_GETINFO_BPFEXT;
oid_data_arg->Length = sizeof(ULONG);
if (PacketGetInfo(pw->adapter, oid_data_arg)) {
if (*((ULONG *)oid_data_arg->Data) >= SKF_AD_VLAN_TAG_PRESENT) {
if (*((ULONG *)oid_data_arg->Data) >= NPCAP_AD_VLAN_TAG_PRESENT) {
/* Yes, we can. Request that we do so. */
p->bpf_codegen_flags |= BPF_SPECIAL_VLAN_HANDLING;
}
Expand Down