[1udfx9cf8azi0]

    if (nbytes < sizeof(*hwrpb))
        return -1;
    
    if (copy_to_user(buffer, hwrpb, nbytes) != 0)
        return -2;

The fix that was done was:

    if (nbytes > sizeof(*hwrpb))

But I think the correct fix is:

    if (copy_to_user(buffer, hwrpb, sizeof(*hwrpb)) != 0)

It never makes sense to copy out of the hwrpb pointer any size other than sizeof(*hwrpb).

[another2another]

If the caller has nbytes = 4

and sizeof(hwrpb) is now 16 bytes, then you will be copying 12 bytes of data too many from the caller, potentially reading into memory it doesn't own. I would say that should be avoided.

The better solution I believe would be to only copy the minimum amount of bytes supported by caller & callee. So:

nbytes = MIN(nbytes, sizeof(hwrpb));

Which should ensure backwards and forwards compatibility, assuming the version info of hwrpb->size is respected then the fact that part of the hwrpb struct isn't initialized shouldn't matter.

[pwagland]

Right, but the size of the buffer is given, it doesn't make sense to stomp over end of the callers buffer either, so you can't use pass in something longer than `nbytes` either.

[1udfx9cf8azi0]

That's what the original check is for:

    if (nbytes < sizeof(*hwrpb))

If the buffer isn't large enough to hold *hwrpb, then it already fails. The original check was good, only needed to change the amount of bytes copied to sizeof(*hwrpb).

[sltkr]

No, because if nbytes > sizeof(*hwrpb), your version causes the kernel to only write part of the buffer, and then when the app accesses fields at the end of the struct, it would read uninitialized data, which is very bad.

Recall that the API is intended to be used like this:

    struct hwrbp buf;
    getsysinfo(GSI_GET_HWRPB, &buf, sizeof(buf), /* .. */);

At first glance, it might seem unnecessary to pass the buffer size at all, because in theory the user and kernel should agree on what the sizeof(struct hwrbp) is. But the reason it is passed is because there are various reasons why the separately compiled kernel and user binaries might disagree (e.g., incorrect compiler flags, wrong header file being used, struct has changed between different versions, etc.), and it's useful to detect that. So you can make an argument that the most conservative check is:

    if (nbytes != sizeof(\*hwrpb)) return -1;

After all, if the user and kernel disagree on the correct size of the struct, then something is wrong! But allowing nbytes < sizeof(*hwrpb) has the benefit that the kernel developers can add fields at the end of the struct without breaking backward compatibility with older applications.

I would agree with you if the kernel had some other mechanism to pass the size of the buffer that was actually filled to the client (like e.g. the read() syscall does) but the getsysinfo() API doesn't return that data, so the kernel must either fill the buffer entirely or return failure.*

[1udfx9cf8azi0]

> No, because if nbytes > sizeof(*hwrpb), your version causes the kernel to only write part of the buffer, and then when the app accesses fields at the end of the struct, it would read uninitialized data, which is very bad.

> I would agree with you if the kernel had some other mechanism to pass the size of the buffer that was actually filled to the client (like e.g. the read() syscall does) but the getsysinfo() API doesn't return that data, so the kernel must either fill the buffer entirely or return failure.

As you mention, this struct is versioned. Userspace can tell how much of the struct was filled by checking the size field (hwrpb->size).

> But allowing nbytes < sizeof(*hwrpb) has the benefit that the kernel developers can add fields at the end of the struct without breaking backward compatibility with older applications.

That's a related but separate issue. Backward compatibility can be handled by switching on nbytes or by copying fewer bytes with a carefully designed struct. It's not clear that backward compatibility was the original intention of this code, the original intention more seems to be sanitizing tainted input. This struct has not changed in at least 16 years.

[tom_]

The original less-than check was deemed incorrect, and was replaced entirely. For good or for ill, it seems the author deems it valid to pass in a value smaller than sizeof *hwrpb, and that many bytes will be dutifully copied. This might form part of some barebones API versioning mechanism.

[1udfx9cf8azi0]

> The original less-than check was deemed incorrect

It was only deemed incorrect because of an information leak. Not because it's a valid use-case for user space to copy smaller portions of *hwrpb into user space. https://github.com/torvalds/linux/commit/21c5977a836e399fc71...