Skip to content
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.

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

librtas: fix buffer length determination in rtas_set_sysparm() #35

Conversation

nathanlynch
Copy link
Contributor

The data passed by callers has this layout:

struct {
uint8_t len_msb;
uint8_t len_lsb;
char payload[];
};

I.e. the length of valid data in payload[] is stored in the first two bytes of the buffer. However, rtas_set_sysparm() wrongly assumes that the length is in host-endian, leading to miscalculation of allocation and copy sizes.

For example, given this code in rtas_set_sysparm():

int rtas_set_sysparm(unsigned int parameter, char *data) {
...
size = *(short *)data;
...
memcpy(kernbuf, data, size + sizeof(short));
...
}

on little-endian targets, the following code:

struct {
    uint16_t len_be;
    char data[1024];
} sp_block = {
    .len_be = htobe16(sizeof"hello"),
    .data = "hello",
};
rtas_set_sysparm(0, (char *)&sp_block);

results in a copy of size ((6 << 8) + 2) = 1538, reading beyond the end of the passed object.

Similarly, objects with an encoded size of 256 or greater will result in copying too little data.

Extract the size from the buffer correctly, always treating it as big-endian.

The data passed by callers has this layout:

  struct {
      uint8_t len_msb;
      uint8_t len_lsb;
      char payload[];
  };

I.e. the length of valid data in payload[] is stored in the first two
bytes of the buffer. However, rtas_set_sysparm() wrongly assumes that
the length is in host-endian, leading to miscalculation of allocation
and copy sizes.

For example, given this code in rtas_set_sysparm():

  int rtas_set_sysparm(unsigned int parameter, char *data) {
      ...
      size = *(short *)data;
      ...
      memcpy(kernbuf, data, size + sizeof(short));
      ...
  }

on little-endian targets, the following code:

    struct {
        uint16_t len_be;
        char data[1024];
    } sp_block = {
        .len_be = htobe16(sizeof"hello"),
        .data = "hello",
    };
    rtas_set_sysparm(0, (char *)&sp_block);

results in a copy of size ((6 << 8) + 2) = 1538, reading beyond the
end of the passed object.

Similarly, objects with an encoded size of 256 or greater will result
in copying too little data.

Extract the size from the buffer correctly, always treating it as
big-endian.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
@nathanlynch nathanlynch marked this pull request as ready for review August 30, 2023 21:07
@nathanlynch nathanlynch requested a review from tyreld September 14, 2023 15:01
@nathanlynch
Copy link
Contributor Author

@tyreld ping? I have a pile of pending development work that depends on this going in first.

Looks like this is necessary after all.

Signed-off-by: Nathan Lynch <nathanl@linux.ibm.com>
@nathanlynch
Copy link
Contributor Author

I've pushed a CI fix on top of this to update the package index in the runners for the qemu jobs.

@tyreld
Copy link
Member

tyreld commented Oct 9, 2023

Merged 0bba0c5 and 8aee694

@tyreld tyreld closed this Oct 9, 2023
@tyreld tyreld added this to the Release 2.0.5 milestone Oct 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants