geom_linux_lvm: Check the offset of physical volume header

[zlei]

Summary

The LVM label are stored on any of the first four sectors, and the
PV (physical volume) header is stored within the same sector following
the LVM label. The current implementation does not fully check the
offset of PV header, when attaching a bad formatted LVM PV the kernel
may crash due to out-of-bounds memory read.

PR: 266562
MFC after: 2 weeks

[jhb]

It would be a lot easier to understand this I think if we had a couple of structs. It would also make this code easier to read:

struct lvm_label_header {
    char sig[8];
    uint64_t sector;
    uint32_t crc;
    uint32_t offset;
    char format[8];
};

/* Starts at `offset` field in label sector. */
struct lvm_label_data {
    char uuid[32];
    uint64_t size;
    uint64_t pestart;
    uint64_t reserved1;
    uint64_t sections;
    uint64_t reserved2;
    uint64_t md_offset;
    uint64_t md_size;
};

Then you could write your expression as if (ll->ll_offset < sizeof(struct lvm_label_header) || ll->ll_offset >= sectorsize - sizeof(struct lvm_label_data)) which would be easier to understand.

It would also permit rewriting much of this function to avoid all the weird off arithmetic (apart from inserting the hyphens in the UUID string).

[zlei]

This is a WIP fix for https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=266562 , I think I should do some refactor as @jhb suggested so it will be much easier to read.

[zlei]

sys/geom/linux_lvm/g_linux_lvm.c

734

I should leave some comments about the hard coded offsets. ;)

[zlei]

Rebased to latest main branch.

[zlei]

1. Fixed off-by-one.
2. Added comment about the hard coded number.

[zlei]

In D36773#941067, @jhb wrote:

It would be a lot easier to understand this I think if we had a couple of structs. It would also make this code easier to read:

struct lvm_label_header {
    char sig[8];
    uint64_t sector;
    uint32_t crc;
    uint32_t offset;
    char format[8];
};

/* Starts at `offset` field in label sector. */
struct lvm_label_data {
    char uuid[32];
    uint64_t size;
    uint64_t pestart;
    uint64_t reserved1;
    uint64_t sections;
    uint64_t reserved2;
    uint64_t md_offset;
    uint64_t md_size;
};

Then you could write your expression as if (ll->ll_offset < sizeof(struct lvm_label_header) || ll->ll_offset >= sectorsize - sizeof(struct lvm_label_data)) which would be easier to understand.

It would also permit rewriting much of this function to avoid all the weird off arithmetic (apart from inserting the hyphens in the UUID string).

For the struct lvm_label_data the actual on-disk layout is not that simple, I'd commit this without refactoring.