| Message ID | 1426008340-24221-1-git-send-email-colin.king@canonical.com |
|---|---|
| State | Rejected |
| Headers | show |
On 15-03-11 01:25 AM, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > BugLink: https://bugs.linaro.org/show_bug.cgi?id=1319 > > We are seeing kernel panics when the EFI userspace is exercising > the efi runtime driver with a zero sized variable. The root cause > is a zero byte kmalloc() which does not return zero and returns > a buffer that we can't actually write into. For zero bytes, allocate > 1 byte buffer so we at least get a legitimate allocation of something. > > Thanks for Naresh Bhat for bisecting the issue and testing. > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > Reviewed/Tested By: Naresh Bhat <naresh.bhat@linaro.org> > --- > efi_runtime/efi_runtime.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/efi_runtime/efi_runtime.c b/efi_runtime/efi_runtime.c > index ff31685..f2ccd57 100644 > --- a/efi_runtime/efi_runtime.c > +++ b/efi_runtime/efi_runtime.c > @@ -154,7 +154,8 @@ copy_ucs2_from_user_len(uint16_t **dst, uint16_t __user *src, size_t len) > if (!access_ok(VERIFY_READ, src, 1)) > return -EFAULT; > > - *dst = kmalloc(len, GFP_KERNEL); > + /* Ensure we don't kmalloc a zero byte buffer */ > + *dst = kmalloc(len ? len : len + 1, GFP_KERNEL); > if (!*dst) > return -ENOMEM; > Acked-by: Alex Hung <alex.hung@canonical.com>
On Tue, 2015-03-10 at 17:25 +0000, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > BugLink: https://bugs.linaro.org/show_bug.cgi?id=1319 > > We are seeing kernel panics when the EFI userspace is exercising > the efi runtime driver with a zero sized variable. The root cause > is a zero byte kmalloc() which does not return zero and returns > a buffer that we can't actually write into. I think that what is happening here is that kmalloc returns ZERO_SIZE_PTR, which is a valid return when doing kmalloc(0), AFAIK. Also, where does it fail exactly? In the subsequent copy_from_user? I wonder why it does not fail on x86? Could it be that ZERO_SIZE_PTR is handled differently on copy_from_user to avoid panics when len=0? > For zero bytes, allocate > 1 byte buffer so we at least get a legitimate allocation of something. Wouldn't this pass artificial data to the firmware? The user specified zero size and yet we are creating a 1-byte buffer. The spirit of de9c23134ac1b5ae5d5a666dd45e10820580dc4e "efi_runtime: get_nextvariable: copy only the needed name bytes" was to not pass artificially-forgiving buffers to the firmware to study its true behavior. What if dst+1 is also '\0'? We will not know if the firmware checks the passed size of the buffer or blindly checks for dst and dst+1 to be '\0' Thanks and BR, Ricardo > > Thanks for Naresh Bhat for bisecting the issue and testing. > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > Reviewed/Tested By: Naresh Bhat <naresh.bhat@linaro.org> > --- > efi_runtime/efi_runtime.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/efi_runtime/efi_runtime.c b/efi_runtime/efi_runtime.c > index ff31685..f2ccd57 100644 > --- a/efi_runtime/efi_runtime.c > +++ b/efi_runtime/efi_runtime.c > @@ -154,7 +154,8 @@ copy_ucs2_from_user_len(uint16_t **dst, uint16_t __user *src, size_t len) > if (!access_ok(VERIFY_READ, src, 1)) > return -EFAULT; > > - *dst = kmalloc(len, GFP_KERNEL); > + /* Ensure we don't kmalloc a zero byte buffer */ > + *dst = kmalloc(len ? len : len + 1, GFP_KERNEL); > if (!*dst) > return -ENOMEM; >
On 11/03/15 01:08, Neri, Ricardo wrote: > On Tue, 2015-03-10 at 17:25 +0000, Colin King wrote: >> From: Colin Ian King <colin.king@canonical.com> >> >> BugLink: https://bugs.linaro.org/show_bug.cgi?id=1319 >> >> We are seeing kernel panics when the EFI userspace is exercising >> the efi runtime driver with a zero sized variable. The root cause >> is a zero byte kmalloc() which does not return zero and returns >> a buffer that we can't actually write into. > > I think that what is happening here is that kmalloc returns > ZERO_SIZE_PTR, which is a valid return when doing kmalloc(0), AFAIK. Yes, looking at this again, it does do that. > Also, where does it fail exactly? In the subsequent copy_from_user? I > wonder why it does not fail on x86? Could it be that ZERO_SIZE_PTR is > handled differently on copy_from_user to avoid panics when len=0? I'll check that out. > >> For zero bytes, allocate >> 1 byte buffer so we at least get a legitimate allocation of something. > > Wouldn't this pass artificial data to the firmware? The user specified > zero size and yet we are creating a 1-byte buffer. The spirit of > de9c23134ac1b5ae5d5a666dd45e10820580dc4e "efi_runtime: get_nextvariable: > copy only the needed name bytes" was to not pass artificially-forgiving > buffers to the firmware to study its true behavior. What if dst+1 is > also '\0'? We will not know if the firmware checks the passed size of > the buffer or blindly checks for dst and dst+1 to be '\0' +1, yep, I've taken that on board and will rethink this. Thanks > > Thanks and BR, > Ricardo >> >> Thanks for Naresh Bhat for bisecting the issue and testing. >> >> Signed-off-by: Colin Ian King <colin.king@canonical.com> >> Reviewed/Tested By: Naresh Bhat <naresh.bhat@linaro.org> >> --- >> efi_runtime/efi_runtime.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/efi_runtime/efi_runtime.c b/efi_runtime/efi_runtime.c >> index ff31685..f2ccd57 100644 >> --- a/efi_runtime/efi_runtime.c >> +++ b/efi_runtime/efi_runtime.c >> @@ -154,7 +154,8 @@ copy_ucs2_from_user_len(uint16_t **dst, uint16_t __user *src, size_t len) >> if (!access_ok(VERIFY_READ, src, 1)) >> return -EFAULT; >> >> - *dst = kmalloc(len, GFP_KERNEL); >> + /* Ensure we don't kmalloc a zero byte buffer */ >> + *dst = kmalloc(len ? len : len + 1, GFP_KERNEL); >> if (!*dst) >> return -ENOMEM; >> >
On 11/03/15 01:08, Neri, Ricardo wrote: > On Tue, 2015-03-10 at 17:25 +0000, Colin King wrote: >> From: Colin Ian King <colin.king@canonical.com> >> >> BugLink: https://bugs.linaro.org/show_bug.cgi?id=1319 >> >> We are seeing kernel panics when the EFI userspace is exercising >> the efi runtime driver with a zero sized variable. The root cause >> is a zero byte kmalloc() which does not return zero and returns >> a buffer that we can't actually write into. > > I think that what is happening here is that kmalloc returns > ZERO_SIZE_PTR, which is a valid return when doing kmalloc(0), AFAIK. > Also, where does it fail exactly? In the subsequent copy_from_user? I > wonder why it does not fail on x86? Could it be that ZERO_SIZE_PTR is > handled differently on copy_from_user to avoid panics when len=0? OK, re-built a clean UEFI enabled aarch64 test VM and an x86 system and compared: aarch64: [ 312.161040] ZERO SIZED PTR: 0000000000000010 [ 312.161252] doing copy_from_user.. [ 312.161406] copy_from_user OK [ 312.161677] calling efi.get_next_variable.. [ 312.162099] Unable to handle kernel NULL pointer dereference at virtual address 00000010 [ 312.162517] pgd = ffffffc03dfb8000 [ 312.162759] [00000010] *pgd=000000007ba00003, *pmd=0000000000000000 [ 312.163622] Internal error: Oops: 94000006 [#1] SMP 1. kmalloc(0) returns ZERO_SIZE_PTR on x86 and aarch64 2. is causes the null ptr deference issue on the efi.get_next_variable call and not the copy_from_user So, I guess the ARM EFI implementation of GetNextVariableName is behaving differently. Is is valid to pass a zero sized buffer into this run time service, I suppose I was expecting a EFI_BUFFER_TOO_SMALL return. However, we are also passing a non-NULL ptr to the name, which will be interpreted as a valid buffer, but in fact it is not. So not sure what to make of this. a) We are passing an incorrect name ptr to the run time service, and it is oopsing on that (which I guess is to be expected given we are passing an invalid buffer over to it). b) We are also passing a valid zero byte length, so perhaps the run time service should return EFI_BUFFER_TOO_SMALL rather than proceeding. Anyhow, I do believe we should pass a valid buffer over rather than ZERO_SIZE_PTR. Colin > >> For zero bytes, allocate >> 1 byte buffer so we at least get a legitimate allocation of something. > > Wouldn't this pass artificial data to the firmware? The user specified > zero size and yet we are creating a 1-byte buffer. The spirit of > de9c23134ac1b5ae5d5a666dd45e10820580dc4e "efi_runtime: get_nextvariable: > copy only the needed name bytes" was to not pass artificially-forgiving > buffers to the firmware to study its true behavior. What if dst+1 is > also '\0'? We will not know if the firmware checks the passed size of > the buffer or blindly checks for dst and dst+1 to be '\0' > > Thanks and BR, > Ricardo >> >> Thanks for Naresh Bhat for bisecting the issue and testing. >> >> Signed-off-by: Colin Ian King <colin.king@canonical.com> >> Reviewed/Tested By: Naresh Bhat <naresh.bhat@linaro.org> >> --- >> efi_runtime/efi_runtime.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/efi_runtime/efi_runtime.c b/efi_runtime/efi_runtime.c >> index ff31685..f2ccd57 100644 >> --- a/efi_runtime/efi_runtime.c >> +++ b/efi_runtime/efi_runtime.c >> @@ -154,7 +154,8 @@ copy_ucs2_from_user_len(uint16_t **dst, uint16_t __user *src, size_t len) >> if (!access_ok(VERIFY_READ, src, 1)) >> return -EFAULT; >> >> - *dst = kmalloc(len, GFP_KERNEL); >> + /* Ensure we don't kmalloc a zero byte buffer */ >> + *dst = kmalloc(len ? len : len + 1, GFP_KERNEL); >> if (!*dst) >> return -ENOMEM; >> >
On Wed, 2015-03-11 at 11:32 +0000, Colin Ian King wrote: > On 11/03/15 01:08, Neri, Ricardo wrote: > > On Tue, 2015-03-10 at 17:25 +0000, Colin King wrote: > >> From: Colin Ian King <colin.king@canonical.com> > >> > >> BugLink: https://bugs.linaro.org/show_bug.cgi?id=1319 > >> > >> We are seeing kernel panics when the EFI userspace is exercising > >> the efi runtime driver with a zero sized variable. The root cause > >> is a zero byte kmalloc() which does not return zero and returns > >> a buffer that we can't actually write into. > > > > I think that what is happening here is that kmalloc returns > > ZERO_SIZE_PTR, which is a valid return when doing kmalloc(0), AFAIK. > > Also, where does it fail exactly? In the subsequent copy_from_user? I > > wonder why it does not fail on x86? Could it be that ZERO_SIZE_PTR is > > handled differently on copy_from_user to avoid panics when len=0? > > OK, re-built a clean UEFI enabled aarch64 test VM and an x86 system and > compared: > > aarch64: > [ 312.161040] ZERO SIZED PTR: 0000000000000010 > [ 312.161252] doing copy_from_user.. > [ 312.161406] copy_from_user OK > [ 312.161677] calling efi.get_next_variable.. > [ 312.162099] Unable to handle kernel NULL pointer dereference at > virtual address 00000010 > [ 312.162517] pgd = ffffffc03dfb8000 > [ 312.162759] [00000010] *pgd=000000007ba00003, *pmd=0000000000000000 > [ 312.163622] Internal error: Oops: 94000006 [#1] SMP > > 1. kmalloc(0) returns ZERO_SIZE_PTR on x86 and aarch64 > 2. is causes the null ptr deference issue on the efi.get_next_variable > call and not the copy_from_user > > So, I guess the ARM EFI implementation of GetNextVariableName is > behaving differently. Is is valid to pass a zero sized buffer into this > run time service, I suppose I was expecting a EFI_BUFFER_TOO_SMALL > return. Yes, I also think this is the expected result according to the spec: "The VariableNameSize is too small for the result." > However, we are also passing a non-NULL ptr to the name, which > will be interpreted as a valid buffer, but in fact it is not. So not > sure what to make of this. I think that ZERO_SIZE_PTR vs NULL are Linux concepts. Thus, maybe the implementation of efi.get_next_variable should translate ZERO_SIZE_PTR into NULL before passing to the firmware? > > a) We are passing an incorrect name ptr to the run time service, and it > is oopsing on that (which I guess is to be expected given we are passing > an invalid buffer over to it). But why efi.get_next_variable needs to dereference the pointer? Page translation maybe? > b) We are also passing a valid zero byte length, so perhaps the run time > service should return EFI_BUFFER_TOO_SMALL rather than proceeding. This should be the behavior if the runtime service has a chance to run. From your results, it seems that Linux is trying to dereference the non-NULL invalid buffer before even passing it to the service. > > Anyhow, I do believe we should pass a valid buffer over rather than > ZERO_SIZE_PTR. Wouldn't this hide the very problem we are discussing here? For instance in drivers/firmware/google/gsmi.c we see the following: static efi_status_t gsmi_get_next_variable(unsigned long *name_size, efi_char16_t *name, efi_guid_t *vendor) { ... /* Let's make sure the thing is at least null-terminated */ if (ucs2_strnlen(name, GSMI_BUF_SIZE / 2) == GSMI_BUF_SIZE / 2) return EFI_INVALID_PARAMETER; ... } The code blindly dereferences name without even checking if it is valid! In order for FWTS to run correctly, perhaps it would be good to put a note saying that this is the approach while the situation is resolved. > Thanks and BR, Ricardo > Colin > > > > > >> For zero bytes, allocate > >> 1 byte buffer so we at least get a legitimate allocation of something. > > > > Wouldn't this pass artificial data to the firmware? The user specified > > zero size and yet we are creating a 1-byte buffer. The spirit of > > de9c23134ac1b5ae5d5a666dd45e10820580dc4e "efi_runtime: get_nextvariable: > > copy only the needed name bytes" was to not pass artificially-forgiving > > buffers to the firmware to study its true behavior. What if dst+1 is > > also '\0'? We will not know if the firmware checks the passed size of > > the buffer or blindly checks for dst and dst+1 to be '\0' > > > > Thanks and BR, > > Ricardo > >> > >> Thanks for Naresh Bhat for bisecting the issue and testing. > >> > >> Signed-off-by: Colin Ian King <colin.king@canonical.com> > >> Reviewed/Tested By: Naresh Bhat <naresh.bhat@linaro.org> > >> --- > >> efi_runtime/efi_runtime.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/efi_runtime/efi_runtime.c b/efi_runtime/efi_runtime.c > >> index ff31685..f2ccd57 100644 > >> --- a/efi_runtime/efi_runtime.c > >> +++ b/efi_runtime/efi_runtime.c > >> @@ -154,7 +154,8 @@ copy_ucs2_from_user_len(uint16_t **dst, uint16_t __user *src, size_t len) > >> if (!access_ok(VERIFY_READ, src, 1)) > >> return -EFAULT; > >> > >> - *dst = kmalloc(len, GFP_KERNEL); > >> + /* Ensure we don't kmalloc a zero byte buffer */ > >> + *dst = kmalloc(len ? len : len + 1, GFP_KERNEL); > >> if (!*dst) > >> return -ENOMEM; > >> > > > >
On 11/03/15 19:04, Ricardo Neri wrote: > On Wed, 2015-03-11 at 11:32 +0000, Colin Ian King wrote: >> On 11/03/15 01:08, Neri, Ricardo wrote: >>> On Tue, 2015-03-10 at 17:25 +0000, Colin King wrote: >>>> From: Colin Ian King <colin.king@canonical.com> >>>> >>>> BugLink: https://bugs.linaro.org/show_bug.cgi?id=1319 >>>> >>>> We are seeing kernel panics when the EFI userspace is exercising >>>> the efi runtime driver with a zero sized variable. The root cause >>>> is a zero byte kmalloc() which does not return zero and returns >>>> a buffer that we can't actually write into. >>> >>> I think that what is happening here is that kmalloc returns >>> ZERO_SIZE_PTR, which is a valid return when doing kmalloc(0), AFAIK. >>> Also, where does it fail exactly? In the subsequent copy_from_user? I >>> wonder why it does not fail on x86? Could it be that ZERO_SIZE_PTR is >>> handled differently on copy_from_user to avoid panics when len=0? >> >> OK, re-built a clean UEFI enabled aarch64 test VM and an x86 system and >> compared: >> >> aarch64: >> [ 312.161040] ZERO SIZED PTR: 0000000000000010 >> [ 312.161252] doing copy_from_user.. >> [ 312.161406] copy_from_user OK >> [ 312.161677] calling efi.get_next_variable.. >> [ 312.162099] Unable to handle kernel NULL pointer dereference at >> virtual address 00000010 >> [ 312.162517] pgd = ffffffc03dfb8000 >> [ 312.162759] [00000010] *pgd=000000007ba00003, *pmd=0000000000000000 >> [ 312.163622] Internal error: Oops: 94000006 [#1] SMP >> >> 1. kmalloc(0) returns ZERO_SIZE_PTR on x86 and aarch64 >> 2. is causes the null ptr deference issue on the efi.get_next_variable >> call and not the copy_from_user >> >> So, I guess the ARM EFI implementation of GetNextVariableName is >> behaving differently. Is is valid to pass a zero sized buffer into this >> run time service, I suppose I was expecting a EFI_BUFFER_TOO_SMALL >> return. > > Yes, I also think this is the expected result according to the spec: > "The VariableNameSize is too small for the result." >> However, we are also passing a non-NULL ptr to the name, which >> will be interpreted as a valid buffer, but in fact it is not. So not >> sure what to make of this. > > I think that ZERO_SIZE_PTR vs NULL are Linux concepts. Thus, maybe the > implementation of efi.get_next_variable should translate ZERO_SIZE_PTR > into NULL before passing to the firmware? Indeed, I agree ZERO_SIZE_PTR is a Linux concept, so we should not use it and the kernel needs to translate it to NULL before passing to the firmware. UEFI should check for a NULL and return EFI_INVALID_PARAMETER if it's doing the right thing. >> >> a) We are passing an incorrect name ptr to the run time service, and it >> is oopsing on that (which I guess is to be expected given we are passing >> an invalid buffer over to it). > > But why efi.get_next_variable needs to dereference the pointer? Page > translation maybe? > >> b) We are also passing a valid zero byte length, so perhaps the run time >> service should return EFI_BUFFER_TOO_SMALL rather than proceeding. > > This should be the behavior if the runtime service has a chance to run. > From your results, it seems that Linux is trying to dereference the > non-NULL invalid buffer before even passing it to the service. That does indeed seem so, but why I can't fathom why that occurs on aarch64 at the present moment. >> >> Anyhow, I do believe we should pass a valid buffer over rather than >> ZERO_SIZE_PTR. > > Wouldn't this hide the very problem we are discussing here? For instance > in drivers/firmware/google/gsmi.c we see the following: > > static efi_status_t gsmi_get_next_variable(unsigned long *name_size, > efi_char16_t *name, > efi_guid_t *vendor) > { > ... > /* Let's make sure the thing is at least null-terminated */ > if (ucs2_strnlen(name, GSMI_BUF_SIZE / 2) == GSMI_BUF_SIZE / 2) > return EFI_INVALID_PARAMETER; > ... > } > > The code blindly dereferences name without even checking if it is valid! Yes, that is a particularly good example of a poor implementation. I think we need to add more tests to fwts to explore invalid run time calls and sanity check that the kernel and the firmware does the right thing. > > > In order for FWTS to run correctly, perhaps it would be good to put a > note saying that this is the approach while the situation is resolved. To clarify, which approach are you referring to? 1) ZERO_SIZE_PTR being replaced to a NULL or 2) allocating a 1 byte buffer and passing that over to stop the kernel breaking. Thanks Colin >> > > Thanks and BR, > Ricardo >> Colin >> >> >>> >>>> For zero bytes, allocate >>>> 1 byte buffer so we at least get a legitimate allocation of something. >>> >>> Wouldn't this pass artificial data to the firmware? The user specified >>> zero size and yet we are creating a 1-byte buffer. The spirit of >>> de9c23134ac1b5ae5d5a666dd45e10820580dc4e "efi_runtime: get_nextvariable: >>> copy only the needed name bytes" was to not pass artificially-forgiving >>> buffers to the firmware to study its true behavior. What if dst+1 is >>> also '\0'? We will not know if the firmware checks the passed size of >>> the buffer or blindly checks for dst and dst+1 to be '\0' >>> >>> Thanks and BR, >>> Ricardo >>>> >>>> Thanks for Naresh Bhat for bisecting the issue and testing. >>>> >>>> Signed-off-by: Colin Ian King <colin.king@canonical.com> >>>> Reviewed/Tested By: Naresh Bhat <naresh.bhat@linaro.org> >>>> --- >>>> efi_runtime/efi_runtime.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/efi_runtime/efi_runtime.c b/efi_runtime/efi_runtime.c >>>> index ff31685..f2ccd57 100644 >>>> --- a/efi_runtime/efi_runtime.c >>>> +++ b/efi_runtime/efi_runtime.c >>>> @@ -154,7 +154,8 @@ copy_ucs2_from_user_len(uint16_t **dst, uint16_t __user *src, size_t len) >>>> if (!access_ok(VERIFY_READ, src, 1)) >>>> return -EFAULT; >>>> >>>> - *dst = kmalloc(len, GFP_KERNEL); >>>> + /* Ensure we don't kmalloc a zero byte buffer */ >>>> + *dst = kmalloc(len ? len : len + 1, GFP_KERNEL); >>>> if (!*dst) >>>> return -ENOMEM; >>>> >>> >> >> > >
On Wed, 2015-03-11 at 19:44 +0000, Colin Ian King wrote: > To clarify, which approach are you referring to? > > 1) ZERO_SIZE_PTR being replaced to a NULL or I would suggest that efi_runtime could make dst = NULL if len = 0, no need to call kmalloc. Hopefully, true NULL cases are handled correctly in efi.get_next_variable and we should be able to see how the firmware handles it. > 2) allocating a 1 byte buffer and passing that over to stop the kernel > breaking. This would be my last resort, provided that the kernel fails only in the ZERO_SIZE_PTR case and not with the NULL case.
On 11/03/15 22:11, Ricardo Neri wrote: > On Wed, 2015-03-11 at 19:44 +0000, Colin Ian King wrote: >> To clarify, which approach are you referring to? >> >> 1) ZERO_SIZE_PTR being replaced to a NULL or > > I would suggest that efi_runtime could make dst = NULL if len = 0, no > need to call kmalloc. Hopefully, true NULL cases are handled correctly > in efi.get_next_variable and we should be able to see how the firmware > handles it. I've checked a handful of implementations on x86 and aarch64, and a NULL causes the service to return EFI_INVALID_PARAMETER, which is to be expected since it is an invalid parameter. In this case, we are now testing the name == NULL validation of the service rather than the len == 0 validation. And doing so means we have changed the semantics of the original test, as it expects EFI_BUFFER_TOO_SMALL to be returned. > >> 2) allocating a 1 byte buffer and passing that over to stop the kernel >> breaking. > > This would be my last resort, provided that the kernel fails only in the > ZERO_SIZE_PTR case and not with the NULL case. > > The alternative is to pass name as a pointer to write protected page for the zero length buffer case, this can then check if the page is being written to (which it should not), the pointer is non-null (hence we don't get the EFI_INVALID_PARAMETER) failure, and hence we can also check for the zero length case. Colin
On Mon, 2015-03-16 at 11:14 +0000, Colin Ian King wrote: > On 11/03/15 22:11, Ricardo Neri wrote: > > On Wed, 2015-03-11 at 19:44 +0000, Colin Ian King wrote: > >> To clarify, which approach are you referring to? > >> > >> 1) ZERO_SIZE_PTR being replaced to a NULL or > > > > I would suggest that efi_runtime could make dst = NULL if len = 0, no > > need to call kmalloc. Hopefully, true NULL cases are handled correctly > > in efi.get_next_variable and we should be able to see how the firmware > > handles it. > > I've checked a handful of implementations on x86 and aarch64, and a NULL > causes the service to return EFI_INVALID_PARAMETER, which is to be > expected since it is an invalid parameter. In this case, we are now > testing the name == NULL validation of the service rather than the len > == 0 validation. And doing so means we have changed the semantics of > the original test, as it expects EFI_BUFFER_TOO_SMALL to be returned. Right. The original test getnextvariable_test4 from the userspace did have a valid buffer but passes a size=0 with the intention of testing the behavior of the firmware. This is the only case in which the actual size of the userspace buffer is different from VariableNameSize. But efi_runtime does not not know the difference and allocates a zero-size buffer and we lose the intention of the test. Your v3 patch cleverly solves the situation :).
diff --git a/efi_runtime/efi_runtime.c b/efi_runtime/efi_runtime.c index ff31685..f2ccd57 100644 --- a/efi_runtime/efi_runtime.c +++ b/efi_runtime/efi_runtime.c @@ -154,7 +154,8 @@ copy_ucs2_from_user_len(uint16_t **dst, uint16_t __user *src, size_t len) if (!access_ok(VERIFY_READ, src, 1)) return -EFAULT; - *dst = kmalloc(len, GFP_KERNEL); + /* Ensure we don't kmalloc a zero byte buffer */ + *dst = kmalloc(len ? len : len + 1, GFP_KERNEL); if (!*dst) return -ENOMEM;