Page MenuHomeFreeBSD

fix malloc length in ip6_output.c:GET_PKTOPT_VAR
Needs ReviewPublic

Authored by jason_eggnet.com on Mar 23 2018, 12:51 AM.

Details

Reviewers
rwatson
sbruno
melifaro
Group Reviewers
transport
network

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 17924
Build 17681: arc lint + arc unit

Event Timeline

additional fixes

@melifaro I'm thinking about how to make GET_PKTOPT_VAR a function or perhaps... how to make it much smaller and call a function for the bulk of the logic.

@melifaro I'm thinking about how to make GET_PKTOPT_VAR a function or perhaps... how to make it much smaller and call a function for the bulk of the logic.

That would be great. The current looks a bit hard to grasp.

sbruno requested changes to this revision.Apr 12 2018, 6:29 PM
This revision now requires changes to proceed.Apr 12 2018, 6:29 PM

update GET_PKTOPT_VAR to properly check for pktopt change after dropping lock

Additionally, refactor a bit for readability.

properly rebase

@melifaro I was not able to refactor into functions but I was able to refactor into something easier to follow and more correct.

I was thinking of having something like the below one as a macro, moving the rest to the actual function.

#define GET_PKTOPT_XVAR(field, max_length) do { \

if (pktopt && pktopt->field) {                                          \
        field_offset = &(pktopt->field) - pktopt;                       \
        error = get_pktopt_var(sopt, in6p, field_offset, &optdata, &optdatalen);\
        if (error != 0)                                                 \
                return (error);                                         \
        malloc_optdata = true;                                          \
}                                                                       \

}

refactor GET_PKTOPT_VAR into a function

@melifaro this is probably what you were thinking?

sbruno accepted this revision.Jul 7 2018, 1:38 PM
This revision is now accepted and ready to land.Jul 7 2018, 1:38 PM
melifaro requested changes to this revision.EditedJul 7 2018, 2:35 PM

Hi Jason,

Not exactly.
My original concern was to make the code more readable and maintainable.
Unfortunately, the new version does not look more readable at all and is twice longer. If there is no way of doing this better - let's just don't touch that macro.

The only thing that really needs to be changed in this part of code is the

optdata = malloc(sopt->sopt_valsize, M_TEMP, M_WAITOK);

since it allocates kernel memory based on user-supplied size argument

This revision now requires changes to proceed.Jul 7 2018, 2:35 PM

Fix GET_PKTOPT_VAR bugs with memory allocation.

  • properly detect and limit memory allocation based on user supplied argument
  • properly detect and recover from requested information length changing after the in6p lock is reacquired

I agree that using less macros didn't particularly help with readability. But there are in fact two problems with the current code. The issue you pointed out with regard to protecting a user space controlled kernel memory allocation, and secondly, properly detecting and recovering from the requested data length changing when the in6p is reacquired.

I believe the now current state of the patch is the correct next step. I also believe that GET_PKTOPT_VAR is the least difficult to follow of all of the evaluated variants with this method.