* [PATCH] rave-sp: Remove VLA
@ 2018-04-23 20:02 Kyle Spiers
2018-04-23 20:08 ` Kees Cook
2018-04-24 5:43 ` Lee Jones
0 siblings, 2 replies; 7+ messages in thread
From: Kyle Spiers @ 2018-04-23 20:02 UTC (permalink / raw)
To: lee.jones; +Cc: linux-kernel, keescook, Kyle Spiers
As part of the effort to remove VLAs from the kernel[1], this creates
constants for the checksum lengths of CCITT and 8B2C and changes
crc_calculated to be the maximum size of a checksum.
https://lkml.org/lkml/2018/3/7/621
Signed-off-by: Kyle Spiers <ksspiers@google.com>
---
drivers/mfd/rave-sp.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/mfd/rave-sp.c b/drivers/mfd/rave-sp.c
index 5c858e784a89..99fa482419f9 100644
--- a/drivers/mfd/rave-sp.c
+++ b/drivers/mfd/rave-sp.c
@@ -45,7 +45,9 @@
#define RAVE_SP_DLE 0x10
#define RAVE_SP_MAX_DATA_SIZE 64
-#define RAVE_SP_CHECKSUM_SIZE 2 /* Worst case scenario on RDU2 */
+#define RAVE_SP_CHECKSUM_8B2C 1
+#define RAVE_SP_CHECKSUM_CCITT 2
+#define RAVE_SP_CHECKSUM_SIZE RAVE_SP_CHECKSUM_CCITT
/*
* We don't store STX, ETX and unescaped bytes, so Rx is only
* DATA + CSUM
@@ -415,7 +417,12 @@ static void rave_sp_receive_frame(struct rave_sp *sp,
const size_t payload_length = length - checksum_length;
const u8 *crc_reported = &data[payload_length];
struct device *dev = &sp->serdev->dev;
- u8 crc_calculated[checksum_length];
+ u8 crc_calculated[RAVE_SP_CHECKSUM_SIZE];
+
+ if (unlikely(length > sizeof(crc_calculated))) {
+ dev_warn(dev, "Dropping oversized frame\n");
+ return;
+ }
print_hex_dump(KERN_DEBUG, "rave-sp rx: ", DUMP_PREFIX_NONE,
16, 1, data, length, false);
--
2.17.0.484.g0c8726318c-goog
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] rave-sp: Remove VLA
2018-04-23 20:02 [PATCH] rave-sp: Remove VLA Kyle Spiers
@ 2018-04-23 20:08 ` Kees Cook
2018-04-24 5:43 ` Lee Jones
1 sibling, 0 replies; 7+ messages in thread
From: Kees Cook @ 2018-04-23 20:08 UTC (permalink / raw)
To: Kyle Spiers; +Cc: Lee Jones, LKML
On Mon, Apr 23, 2018 at 1:02 PM, Kyle Spiers <ksspiers@google.com> wrote:
> As part of the effort to remove VLAs from the kernel[1], this creates
> constants for the checksum lengths of CCITT and 8B2C and changes
> crc_calculated to be the maximum size of a checksum.
>
> https://lkml.org/lkml/2018/3/7/621
>
> Signed-off-by: Kyle Spiers <ksspiers@google.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
> drivers/mfd/rave-sp.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mfd/rave-sp.c b/drivers/mfd/rave-sp.c
> index 5c858e784a89..99fa482419f9 100644
> --- a/drivers/mfd/rave-sp.c
> +++ b/drivers/mfd/rave-sp.c
> @@ -45,7 +45,9 @@
> #define RAVE_SP_DLE 0x10
>
> #define RAVE_SP_MAX_DATA_SIZE 64
> -#define RAVE_SP_CHECKSUM_SIZE 2 /* Worst case scenario on RDU2 */
> +#define RAVE_SP_CHECKSUM_8B2C 1
> +#define RAVE_SP_CHECKSUM_CCITT 2
> +#define RAVE_SP_CHECKSUM_SIZE RAVE_SP_CHECKSUM_CCITT
> /*
> * We don't store STX, ETX and unescaped bytes, so Rx is only
> * DATA + CSUM
> @@ -415,7 +417,12 @@ static void rave_sp_receive_frame(struct rave_sp *sp,
> const size_t payload_length = length - checksum_length;
> const u8 *crc_reported = &data[payload_length];
> struct device *dev = &sp->serdev->dev;
> - u8 crc_calculated[checksum_length];
> + u8 crc_calculated[RAVE_SP_CHECKSUM_SIZE];
> +
> + if (unlikely(length > sizeof(crc_calculated))) {
> + dev_warn(dev, "Dropping oversized frame\n");
> + return;
> + }
>
> print_hex_dump(KERN_DEBUG, "rave-sp rx: ", DUMP_PREFIX_NONE,
> 16, 1, data, length, false);
> --
> 2.17.0.484.g0c8726318c-goog
>
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] rave-sp: Remove VLA
2018-04-23 20:02 [PATCH] rave-sp: Remove VLA Kyle Spiers
2018-04-23 20:08 ` Kees Cook
@ 2018-04-24 5:43 ` Lee Jones
2018-04-24 16:07 ` Kees Cook
1 sibling, 1 reply; 7+ messages in thread
From: Lee Jones @ 2018-04-24 5:43 UTC (permalink / raw)
To: Kyle Spiers; +Cc: linux-kernel, keescook
On Mon, 23 Apr 2018, Kyle Spiers wrote:
> As part of the effort to remove VLAs from the kernel[1], this creates
> constants for the checksum lengths of CCITT and 8B2C and changes
> crc_calculated to be the maximum size of a checksum.
>
> https://lkml.org/lkml/2018/3/7/621
>
> Signed-off-by: Kyle Spiers <ksspiers@google.com>
> ---
> drivers/mfd/rave-sp.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mfd/rave-sp.c b/drivers/mfd/rave-sp.c
> index 5c858e784a89..99fa482419f9 100644
> --- a/drivers/mfd/rave-sp.c
> +++ b/drivers/mfd/rave-sp.c
> @@ -45,7 +45,9 @@
> #define RAVE_SP_DLE 0x10
>
> #define RAVE_SP_MAX_DATA_SIZE 64
> -#define RAVE_SP_CHECKSUM_SIZE 2 /* Worst case scenario on RDU2 */
> +#define RAVE_SP_CHECKSUM_8B2C 1
> +#define RAVE_SP_CHECKSUM_CCITT 2
> +#define RAVE_SP_CHECKSUM_SIZE RAVE_SP_CHECKSUM_CCITT
> /*
> * We don't store STX, ETX and unescaped bytes, so Rx is only
> * DATA + CSUM
> @@ -415,7 +417,12 @@ static void rave_sp_receive_frame(struct rave_sp *sp,
> const size_t payload_length = length - checksum_length;
> const u8 *crc_reported = &data[payload_length];
> struct device *dev = &sp->serdev->dev;
> - u8 crc_calculated[checksum_length];
> + u8 crc_calculated[RAVE_SP_CHECKSUM_SIZE];
> +
> + if (unlikely(length > sizeof(crc_calculated))) {
Forgive me if I have this wrong (it's still very early here), but this
doesn't leave any room for the payload?
<-- length -->
<ck len><- payload length ->
[CK][CK][D][A][T][A] .. [64]
It is my hope that length would always be larger than the size of the
checksum, or else there would never be any data?
Should this not be:
if (unlikely(length > RAVE_SP_MAX_DATA_SIZE))
Nit: Adding the check is also an unrelated change, so would require a
separate patch.
> + dev_warn(dev, "Dropping oversized frame\n");
> + return;
> + }
>
> print_hex_dump(KERN_DEBUG, "rave-sp rx: ", DUMP_PREFIX_NONE,
> 16, 1, data, length, false);
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] rave-sp: Remove VLA
2018-04-24 5:43 ` Lee Jones
@ 2018-04-24 16:07 ` Kees Cook
2018-04-25 10:31 ` Lee Jones
0 siblings, 1 reply; 7+ messages in thread
From: Kees Cook @ 2018-04-24 16:07 UTC (permalink / raw)
To: Lee Jones; +Cc: Kyle Spiers, LKML
On Mon, Apr 23, 2018 at 10:43 PM, Lee Jones <lee.jones@linaro.org> wrote:
> On Mon, 23 Apr 2018, Kyle Spiers wrote:
>
>> As part of the effort to remove VLAs from the kernel[1], this creates
>> constants for the checksum lengths of CCITT and 8B2C and changes
>> crc_calculated to be the maximum size of a checksum.
>>
>> https://lkml.org/lkml/2018/3/7/621
>>
>> Signed-off-by: Kyle Spiers <ksspiers@google.com>
>> ---
>> drivers/mfd/rave-sp.c | 11 +++++++++--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mfd/rave-sp.c b/drivers/mfd/rave-sp.c
>> index 5c858e784a89..99fa482419f9 100644
>> --- a/drivers/mfd/rave-sp.c
>> +++ b/drivers/mfd/rave-sp.c
>> @@ -45,7 +45,9 @@
>> #define RAVE_SP_DLE 0x10
>>
>> #define RAVE_SP_MAX_DATA_SIZE 64
>> -#define RAVE_SP_CHECKSUM_SIZE 2 /* Worst case scenario on RDU2 */
>> +#define RAVE_SP_CHECKSUM_8B2C 1
>> +#define RAVE_SP_CHECKSUM_CCITT 2
>> +#define RAVE_SP_CHECKSUM_SIZE RAVE_SP_CHECKSUM_CCITT
>> /*
>> * We don't store STX, ETX and unescaped bytes, so Rx is only
>> * DATA + CSUM
>> @@ -415,7 +417,12 @@ static void rave_sp_receive_frame(struct rave_sp *sp,
>> const size_t payload_length = length - checksum_length;
>> const u8 *crc_reported = &data[payload_length];
>> struct device *dev = &sp->serdev->dev;
>> - u8 crc_calculated[checksum_length];
>> + u8 crc_calculated[RAVE_SP_CHECKSUM_SIZE];
>> +
>> + if (unlikely(length > sizeof(crc_calculated))) {
>
> Forgive me if I have this wrong (it's still very early here), but this
> doesn't leave any room for the payload?
>
> <-- length -->
> <ck len><- payload length ->
> [CK][CK][D][A][T][A] .. [64]
>
> It is my hope that length would always be larger than the size of the
> checksum, or else there would never be any data?
>
> Should this not be:
>
> if (unlikely(length > RAVE_SP_MAX_DATA_SIZE))
Oh, whoops, no, this should be:
+ if (unlikely(checksum_lengh > sizeof(crc_calculated))) {
(To validate the VLA max size.)
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] rave-sp: Remove VLA
2018-04-24 16:07 ` Kees Cook
@ 2018-04-25 10:31 ` Lee Jones
2018-04-25 16:51 ` Kyle Spiers
0 siblings, 1 reply; 7+ messages in thread
From: Lee Jones @ 2018-04-25 10:31 UTC (permalink / raw)
To: Kees Cook; +Cc: Kyle Spiers, LKML
On Tue, 24 Apr 2018, Kees Cook wrote:
> On Mon, Apr 23, 2018 at 10:43 PM, Lee Jones <lee.jones@linaro.org> wrote:
> > On Mon, 23 Apr 2018, Kyle Spiers wrote:
> >
> >> As part of the effort to remove VLAs from the kernel[1], this creates
> >> constants for the checksum lengths of CCITT and 8B2C and changes
> >> crc_calculated to be the maximum size of a checksum.
> >>
> >> https://lkml.org/lkml/2018/3/7/621
> >>
> >> Signed-off-by: Kyle Spiers <ksspiers@google.com>
> >> ---
> >> drivers/mfd/rave-sp.c | 11 +++++++++--
> >> 1 file changed, 9 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/mfd/rave-sp.c b/drivers/mfd/rave-sp.c
> >> index 5c858e784a89..99fa482419f9 100644
> >> --- a/drivers/mfd/rave-sp.c
> >> +++ b/drivers/mfd/rave-sp.c
> >> @@ -45,7 +45,9 @@
> >> #define RAVE_SP_DLE 0x10
> >>
> >> #define RAVE_SP_MAX_DATA_SIZE 64
> >> -#define RAVE_SP_CHECKSUM_SIZE 2 /* Worst case scenario on RDU2 */
> >> +#define RAVE_SP_CHECKSUM_8B2C 1
> >> +#define RAVE_SP_CHECKSUM_CCITT 2
> >> +#define RAVE_SP_CHECKSUM_SIZE RAVE_SP_CHECKSUM_CCITT
> >> /*
> >> * We don't store STX, ETX and unescaped bytes, so Rx is only
> >> * DATA + CSUM
> >> @@ -415,7 +417,12 @@ static void rave_sp_receive_frame(struct rave_sp *sp,
> >> const size_t payload_length = length - checksum_length;
> >> const u8 *crc_reported = &data[payload_length];
> >> struct device *dev = &sp->serdev->dev;
> >> - u8 crc_calculated[checksum_length];
> >> + u8 crc_calculated[RAVE_SP_CHECKSUM_SIZE];
> >> +
> >> + if (unlikely(length > sizeof(crc_calculated))) {
> >
> > Forgive me if I have this wrong (it's still very early here), but this
> > doesn't leave any room for the payload?
> >
> > <-- length -->
> > <ck len><- payload length ->
> > [CK][CK][D][A][T][A] .. [64]
> >
> > It is my hope that length would always be larger than the size of the
> > checksum, or else there would never be any data?
> >
> > Should this not be:
> >
> > if (unlikely(length > RAVE_SP_MAX_DATA_SIZE))
>
> Oh, whoops, no, this should be:
>
> + if (unlikely(checksum_lengh > sizeof(crc_calculated))) {
>
> (To validate the VLA max size.)
That doesn't match the OP's error message though:
dev_warn(dev, "Dropping oversized frame\n");
Which I assume is designed to complement the existing warning:
if (unlikely(length <= checksum_length))
dev_warn(dev, "Dropping short frame\n");
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] rave-sp: Remove VLA
2018-04-25 10:31 ` Lee Jones
@ 2018-04-25 16:51 ` Kyle Spiers
2018-04-26 7:44 ` Lee Jones
0 siblings, 1 reply; 7+ messages in thread
From: Kyle Spiers @ 2018-04-25 16:51 UTC (permalink / raw)
To: Lee Jones; +Cc: Kees Cook, linux-kernel
The error message is also wrong. Would "Checksum length too large" be fine?
On Wed, Apr 25, 2018 at 3:31 AM Lee Jones <lee.jones@linaro.org> wrote:
> On Tue, 24 Apr 2018, Kees Cook wrote:
> > On Mon, Apr 23, 2018 at 10:43 PM, Lee Jones <lee.jones@linaro.org>
wrote:
> > > On Mon, 23 Apr 2018, Kyle Spiers wrote:
> > >
> > >> As part of the effort to remove VLAs from the kernel[1], this creates
> > >> constants for the checksum lengths of CCITT and 8B2C and changes
> > >> crc_calculated to be the maximum size of a checksum.
> > >>
> > >> https://lkml.org/lkml/2018/3/7/621
> > >>
> > >> Signed-off-by: Kyle Spiers <ksspiers@google.com>
> > >> ---
> > >> drivers/mfd/rave-sp.c | 11 +++++++++--
> > >> 1 file changed, 9 insertions(+), 2 deletions(-)
> > >>
> > >> diff --git a/drivers/mfd/rave-sp.c b/drivers/mfd/rave-sp.c
> > >> index 5c858e784a89..99fa482419f9 100644
> > >> --- a/drivers/mfd/rave-sp.c
> > >> +++ b/drivers/mfd/rave-sp.c
> > >> @@ -45,7 +45,9 @@
> > >> #define RAVE_SP_DLE 0x10
> > >>
> > >> #define RAVE_SP_MAX_DATA_SIZE 64
> > >> -#define RAVE_SP_CHECKSUM_SIZE 2 /* Worst case
scenario on RDU2 */
> > >> +#define RAVE_SP_CHECKSUM_8B2C 1
> > >> +#define RAVE_SP_CHECKSUM_CCITT 2
> > >> +#define RAVE_SP_CHECKSUM_SIZE RAVE_SP_CHECKSUM_CCITT
> > >> /*
> > >> * We don't store STX, ETX and unescaped bytes, so Rx is only
> > >> * DATA + CSUM
> > >> @@ -415,7 +417,12 @@ static void rave_sp_receive_frame(struct
rave_sp *sp,
> > >> const size_t payload_length = length - checksum_length;
> > >> const u8 *crc_reported = &data[payload_length];
> > >> struct device *dev = &sp->serdev->dev;
> > >> - u8 crc_calculated[checksum_length];
> > >> + u8 crc_calculated[RAVE_SP_CHECKSUM_SIZE];
> > >> +
> > >> + if (unlikely(length > sizeof(crc_calculated))) {
> > >
> > > Forgive me if I have this wrong (it's still very early here), but this
> > > doesn't leave any room for the payload?
> > >
> > > <-- length -->
> > > <ck len><- payload length ->
> > > [CK][CK][D][A][T][A] .. [64]
> > >
> > > It is my hope that length would always be larger than the size of the
> > > checksum, or else there would never be any data?
> > >
> > > Should this not be:
> > >
> > > if (unlikely(length > RAVE_SP_MAX_DATA_SIZE))
> >
> > Oh, whoops, no, this should be:
> >
> > + if (unlikely(checksum_lengh > sizeof(crc_calculated))) {
> >
> > (To validate the VLA max size.)
> That doesn't match the OP's error message though:
> dev_warn(dev, "Dropping oversized frame\n");
> Which I assume is designed to complement the existing warning:
> if (unlikely(length <= checksum_length))
> dev_warn(dev, "Dropping short frame\n");
> --
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] rave-sp: Remove VLA
2018-04-25 16:51 ` Kyle Spiers
@ 2018-04-26 7:44 ` Lee Jones
0 siblings, 0 replies; 7+ messages in thread
From: Lee Jones @ 2018-04-26 7:44 UTC (permalink / raw)
To: Kyle Spiers; +Cc: Kees Cook, linux-kernel
> On Wed, Apr 25, 2018 at 3:31 AM Lee Jones <lee.jones@linaro.org> wrote:
>
> > On Tue, 24 Apr 2018, Kees Cook wrote:
>
> > > On Mon, Apr 23, 2018 at 10:43 PM, Lee Jones <lee.jones@linaro.org>
> wrote:
> > > > On Mon, 23 Apr 2018, Kyle Spiers wrote:
> > > >
> > > >> As part of the effort to remove VLAs from the kernel[1], this creates
> > > >> constants for the checksum lengths of CCITT and 8B2C and changes
> > > >> crc_calculated to be the maximum size of a checksum.
> > > >>
> > > >> https://lkml.org/lkml/2018/3/7/621
> > > >>
> > > >> Signed-off-by: Kyle Spiers <ksspiers@google.com>
> > > >> ---
> > > >> drivers/mfd/rave-sp.c | 11 +++++++++--
> > > >> 1 file changed, 9 insertions(+), 2 deletions(-)
> > > >>
> > > >> diff --git a/drivers/mfd/rave-sp.c b/drivers/mfd/rave-sp.c
> > > >> index 5c858e784a89..99fa482419f9 100644
> > > >> --- a/drivers/mfd/rave-sp.c
> > > >> +++ b/drivers/mfd/rave-sp.c
> > > >> @@ -45,7 +45,9 @@
> > > >> #define RAVE_SP_DLE 0x10
> > > >>
> > > >> #define RAVE_SP_MAX_DATA_SIZE 64
> > > >> -#define RAVE_SP_CHECKSUM_SIZE 2 /* Worst case
> scenario on RDU2 */
> > > >> +#define RAVE_SP_CHECKSUM_8B2C 1
> > > >> +#define RAVE_SP_CHECKSUM_CCITT 2
> > > >> +#define RAVE_SP_CHECKSUM_SIZE RAVE_SP_CHECKSUM_CCITT
> > > >> /*
> > > >> * We don't store STX, ETX and unescaped bytes, so Rx is only
> > > >> * DATA + CSUM
> > > >> @@ -415,7 +417,12 @@ static void rave_sp_receive_frame(struct
> rave_sp *sp,
> > > >> const size_t payload_length = length - checksum_length;
> > > >> const u8 *crc_reported = &data[payload_length];
> > > >> struct device *dev = &sp->serdev->dev;
> > > >> - u8 crc_calculated[checksum_length];
> > > >> + u8 crc_calculated[RAVE_SP_CHECKSUM_SIZE];
> > > >> +
> > > >> + if (unlikely(length > sizeof(crc_calculated))) {
> > > >
> > > > Forgive me if I have this wrong (it's still very early here), but this
> > > > doesn't leave any room for the payload?
> > > >
> > > > <-- length -->
> > > > <ck len><- payload length ->
> > > > [CK][CK][D][A][T][A] .. [64]
> > > >
> > > > It is my hope that length would always be larger than the size of the
> > > > checksum, or else there would never be any data?
> > > >
> > > > Should this not be:
> > > >
> > > > if (unlikely(length > RAVE_SP_MAX_DATA_SIZE))
> > >
> > > Oh, whoops, no, this should be:
> > >
> > > + if (unlikely(checksum_lengh > sizeof(crc_calculated))) {
> > >
> > > (To validate the VLA max size.)
>
> > That doesn't match the OP's error message though:
>
> > dev_warn(dev, "Dropping oversized frame\n");
>
> > Which I assume is designed to complement the existing warning:
>
> > if (unlikely(length <= checksum_length))
> > dev_warn(dev, "Dropping short frame\n");
[MOVING - Please don't top post - reply in-line]
> The error message is also wrong. Would "Checksum length too large" be fine?
"Checksum too long, dropping" ?
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-04-26 7:44 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-23 20:02 [PATCH] rave-sp: Remove VLA Kyle Spiers
2018-04-23 20:08 ` Kees Cook
2018-04-24 5:43 ` Lee Jones
2018-04-24 16:07 ` Kees Cook
2018-04-25 10:31 ` Lee Jones
2018-04-25 16:51 ` Kyle Spiers
2018-04-26 7:44 ` Lee Jones
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.