These structs have holes in them so we end up disclosing a few bytes of
uninitialized stack data.
drivers/misc/xilinx_sdfec.c:305 xsdfec_get_status() warn: check that 'status' doesn't leak information (struct has a hole after 'activity')
drivers/misc/xilinx_sdfec.c:449 xsdfec_get_turbo() warn: check that 'turbo_params' doesn't leak information (struct has a hole after 'scale')
We need to zero out the holes with memset().
Fixes: 6bd6a690c2e7 ("misc: xilinx_sdfec: Add stats & status ioctls")
Signed-off-by: Dan Carpenter <[email protected]>
---
drivers/misc/xilinx_sdfec.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/misc/xilinx_sdfec.c b/drivers/misc/xilinx_sdfec.c
index 912e939dec62..dc1b8b412712 100644
--- a/drivers/misc/xilinx_sdfec.c
+++ b/drivers/misc/xilinx_sdfec.c
@@ -295,6 +295,7 @@ static int xsdfec_get_status(struct xsdfec_dev *xsdfec, void __user *arg)
struct xsdfec_status status;
int err;
+ memset(&status, 0, sizeof(status));
spin_lock_irqsave(&xsdfec->error_data_lock, xsdfec->flags);
status.state = xsdfec->state;
xsdfec->state_updated = false;
@@ -440,6 +441,7 @@ static int xsdfec_get_turbo(struct xsdfec_dev *xsdfec, void __user *arg)
if (xsdfec->config.code == XSDFEC_LDPC_CODE)
return -EIO;
+ memset(&turbo_params, 0, sizeof(turbo_params));
reg_value = xsdfec_regread(xsdfec, XSDFEC_TURBO_ADDR);
turbo_params.scale = (reg_value & XSDFEC_TURBO_SCALE_MASK) >>
--
2.20.1
The checking here needs to handle integer overflows because "offset" and
"len" come from the user.
Fixes: 20ec628e8007 ("misc: xilinx_sdfec: Add ability to configure LDPC")
Signed-off-by: Dan Carpenter <[email protected]>
---
drivers/misc/xilinx_sdfec.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/misc/xilinx_sdfec.c b/drivers/misc/xilinx_sdfec.c
index 3fc53d20abf3..0bf3bcc8e1ef 100644
--- a/drivers/misc/xilinx_sdfec.c
+++ b/drivers/misc/xilinx_sdfec.c
@@ -611,7 +611,9 @@ static int xsdfec_table_write(struct xsdfec_dev *xsdfec, u32 offset,
* Writes that go beyond the length of
* Shared Scale(SC) table should fail
*/
- if ((XSDFEC_REG_WIDTH_JUMP * (offset + len)) > depth) {
+ if (offset > depth / XSDFEC_REG_WIDTH_JUMP ||
+ len > depth / XSDFEC_REG_WIDTH_JUMP ||
+ offset + len > depth / XSDFEC_REG_WIDTH_JUMP) {
dev_dbg(xsdfec->dev, "Write exceeds SC table length");
return -EINVAL;
}
--
2.20.1
Hi Dan,
On 21. 08. 19 9:06, Dan Carpenter wrote:
> These structs have holes in them so we end up disclosing a few bytes of
> uninitialized stack data.
>
> drivers/misc/xilinx_sdfec.c:305 xsdfec_get_status() warn: check that 'status' doesn't leak information (struct has a hole after 'activity')
> drivers/misc/xilinx_sdfec.c:449 xsdfec_get_turbo() warn: check that 'turbo_params' doesn't leak information (struct has a hole after 'scale')
Who is generating these warnings? Is this any new GCC or different tool?
I see that 3byte padding but never seen these warnings.
> We need to zero out the holes with memset().
>
> Fixes: 6bd6a690c2e7 ("misc: xilinx_sdfec: Add stats & status ioctls")
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> drivers/misc/xilinx_sdfec.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/misc/xilinx_sdfec.c b/drivers/misc/xilinx_sdfec.c
> index 912e939dec62..dc1b8b412712 100644
> --- a/drivers/misc/xilinx_sdfec.c
> +++ b/drivers/misc/xilinx_sdfec.c
> @@ -295,6 +295,7 @@ static int xsdfec_get_status(struct xsdfec_dev *xsdfec, void __user *arg)
> struct xsdfec_status status;
> int err;
>
> + memset(&status, 0, sizeof(status));
> spin_lock_irqsave(&xsdfec->error_data_lock, xsdfec->flags);
> status.state = xsdfec->state;
> xsdfec->state_updated = false;
> @@ -440,6 +441,7 @@ static int xsdfec_get_turbo(struct xsdfec_dev *xsdfec, void __user *arg)
> if (xsdfec->config.code == XSDFEC_LDPC_CODE)
> return -EIO;
>
> + memset(&turbo_params, 0, sizeof(turbo_params));
> reg_value = xsdfec_regread(xsdfec, XSDFEC_TURBO_ADDR);
>
> turbo_params.scale = (reg_value & XSDFEC_TURBO_SCALE_MASK) >>
>
Reviewed-by: Michal Simek <[email protected]>
Thanks,
Michal
On 22. 08. 19 10:28, Dan Carpenter wrote:
> On Thu, Aug 22, 2019 at 10:14:12AM +0200, Michal Simek wrote:
>> Hi Dan,
>>
>> On 21. 08. 19 9:06, Dan Carpenter wrote:
>>> These structs have holes in them so we end up disclosing a few bytes of
>>> uninitialized stack data.
>>>
>>> drivers/misc/xilinx_sdfec.c:305 xsdfec_get_status() warn: check that 'status' doesn't leak information (struct has a hole after 'activity')
>>> drivers/misc/xilinx_sdfec.c:449 xsdfec_get_turbo() warn: check that 'turbo_params' doesn't leak information (struct has a hole after 'scale')
>>
>> Who is generating these warnings? Is this any new GCC or different tool?
>> I see that 3byte padding but never seen these warnings.
>
> This is a Smatch check.
ok. It looks like I need to update it to latest version. My version is
not showing these.
Anyway thanks for patches,
Michal
On 21. 08. 19 9:11, Dan Carpenter wrote:
> The checking here needs to handle integer overflows because "offset" and
> "len" come from the user.
>
> Fixes: 20ec628e8007 ("misc: xilinx_sdfec: Add ability to configure LDPC")
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> drivers/misc/xilinx_sdfec.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/misc/xilinx_sdfec.c b/drivers/misc/xilinx_sdfec.c
> index 3fc53d20abf3..0bf3bcc8e1ef 100644
> --- a/drivers/misc/xilinx_sdfec.c
> +++ b/drivers/misc/xilinx_sdfec.c
> @@ -611,7 +611,9 @@ static int xsdfec_table_write(struct xsdfec_dev *xsdfec, u32 offset,
> * Writes that go beyond the length of
> * Shared Scale(SC) table should fail
> */
> - if ((XSDFEC_REG_WIDTH_JUMP * (offset + len)) > depth) {
> + if (offset > depth / XSDFEC_REG_WIDTH_JUMP ||
> + len > depth / XSDFEC_REG_WIDTH_JUMP ||
> + offset + len > depth / XSDFEC_REG_WIDTH_JUMP) {
> dev_dbg(xsdfec->dev, "Write exceeds SC table length");
> return -EINVAL;
> }
>
Reviewed-by: Michal Simek <[email protected]>
Thanks,
Michal
On Thu, Aug 22, 2019 at 10:14:12AM +0200, Michal Simek wrote:
> Hi Dan,
>
> On 21. 08. 19 9:06, Dan Carpenter wrote:
> > These structs have holes in them so we end up disclosing a few bytes of
> > uninitialized stack data.
> >
> > drivers/misc/xilinx_sdfec.c:305 xsdfec_get_status() warn: check that 'status' doesn't leak information (struct has a hole after 'activity')
> > drivers/misc/xilinx_sdfec.c:449 xsdfec_get_turbo() warn: check that 'turbo_params' doesn't leak information (struct has a hole after 'scale')
>
> Who is generating these warnings? Is this any new GCC or different tool?
> I see that 3byte padding but never seen these warnings.
This is a Smatch check.
regards,
dan carpenter
Hi Dan,
> -----Original Message-----
> From: Dan Carpenter [mailto:[email protected]]
> Sent: Wednesday 21 August 2019 08:11
> To: Derek Kiernan <[email protected]>; Dragan Cvetic <[email protected]>
> Cc: Arnd Bergmann <[email protected]>; Greg Kroah-Hartman <[email protected]>; Michal Simek <[email protected]>;
> [email protected]; [email protected]; [email protected]
> Subject: [PATCH 4/4] misc: xilinx_sdfec: Prevent integer overflow in xsdfec_table_write()
>
> The checking here needs to handle integer overflows because "offset" and
> "len" come from the user.
Good catch, thanks.
>
> Fixes: 20ec628e8007 ("misc: xilinx_sdfec: Add ability to configure LDPC")
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> drivers/misc/xilinx_sdfec.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/misc/xilinx_sdfec.c b/drivers/misc/xilinx_sdfec.c
> index 3fc53d20abf3..0bf3bcc8e1ef 100644
> --- a/drivers/misc/xilinx_sdfec.c
> +++ b/drivers/misc/xilinx_sdfec.c
> @@ -611,7 +611,9 @@ static int xsdfec_table_write(struct xsdfec_dev *xsdfec, u32 offset,
> * Writes that go beyond the length of
> * Shared Scale(SC) table should fail
> */
> - if ((XSDFEC_REG_WIDTH_JUMP * (offset + len)) > depth) {
> + if (offset > depth / XSDFEC_REG_WIDTH_JUMP ||
> + len > depth / XSDFEC_REG_WIDTH_JUMP ||
> + offset + len > depth / XSDFEC_REG_WIDTH_JUMP) {
> dev_dbg(xsdfec->dev, "Write exceeds SC table length");
> return -EINVAL;
> }
> --
> 2.20.1
Reviewed-by: Dragan Cvetic <[email protected]>
Thanks
Dragan
Hi Dan,
> -----Original Message-----
> From: Dan Carpenter [mailto:[email protected]]
> Sent: Wednesday 21 August 2019 08:06
> To: Derek Kiernan <[email protected]>; Dragan Cvetic <[email protected]>
> Cc: Arnd Bergmann <[email protected]>; Greg Kroah-Hartman <[email protected]>; Michal Simek <[email protected]>;
> [email protected]; [email protected]; [email protected]
> Subject: [PATCH 1/4] misc: xilinx_sdfec: Fix a couple small information leaks
>
> These structs have holes in them so we end up disclosing a few bytes of
> uninitialized stack data.
>
> drivers/misc/xilinx_sdfec.c:305 xsdfec_get_status() warn: check that 'status' doesn't leak information (struct has a hole after 'activity')
> drivers/misc/xilinx_sdfec.c:449 xsdfec_get_turbo() warn: check that 'turbo_params' doesn't leak information (struct has a hole after
> 'scale')
>
> We need to zero out the holes with memset().
>
> Fixes: 6bd6a690c2e7 ("misc: xilinx_sdfec: Add stats & status ioctls")
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> drivers/misc/xilinx_sdfec.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/misc/xilinx_sdfec.c b/drivers/misc/xilinx_sdfec.c
> index 912e939dec62..dc1b8b412712 100644
> --- a/drivers/misc/xilinx_sdfec.c
> +++ b/drivers/misc/xilinx_sdfec.c
> @@ -295,6 +295,7 @@ static int xsdfec_get_status(struct xsdfec_dev *xsdfec, void __user *arg)
> struct xsdfec_status status;
> int err;
>
> + memset(&status, 0, sizeof(status));
> spin_lock_irqsave(&xsdfec->error_data_lock, xsdfec->flags);
> status.state = xsdfec->state;
> xsdfec->state_updated = false;
> @@ -440,6 +441,7 @@ static int xsdfec_get_turbo(struct xsdfec_dev *xsdfec, void __user *arg)
> if (xsdfec->config.code == XSDFEC_LDPC_CODE)
> return -EIO;
>
> + memset(&turbo_params, 0, sizeof(turbo_params));
> reg_value = xsdfec_regread(xsdfec, XSDFEC_TURBO_ADDR);
>
> turbo_params.scale = (reg_value & XSDFEC_TURBO_SCALE_MASK) >>
> --
> 2.20.1
Reviewed-by: Dragan Cvetic <[email protected]>
Thanks,
Dragan