linux-crypto.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 0/2] Remove BUG_ON() and fix -ve array indexing
@ 2020-03-17  6:15 Rayagonda Kokatanur
  2020-03-17  6:15 ` [PATCH v1 1/2] async_tx: return error instead of BUG_ON Rayagonda Kokatanur
  2020-03-17  6:15 ` [PATCH v1 2/2] async_tx: fix possible negative array indexing Rayagonda Kokatanur
  0 siblings, 2 replies; 7+ messages in thread
From: Rayagonda Kokatanur @ 2020-03-17  6:15 UTC (permalink / raw)
  To: Dan Williams, Herbert Xu, David S . Miller, Allison Randal,
	Kate Stewart, Thomas Gleixner, Greg Kroah-Hartman, linux-crypto,
	linux-kernel
  Cc: Rayagonda Kokatanur

This patch series contains following changes,

1. Avoid use of BUG_ON to prevent kernel crash and return error instead.
2. Fix possible negative array indexing

Rayagonda Kokatanur (2):
  async_tx: return error instead of BUG_ON
  async_tx: fix possible negative array indexing

 crypto/async_tx/async_raid6_recov.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v1 1/2] async_tx: return error instead of BUG_ON
  2020-03-17  6:15 [PATCH v1 0/2] Remove BUG_ON() and fix -ve array indexing Rayagonda Kokatanur
@ 2020-03-17  6:15 ` Rayagonda Kokatanur
  2020-03-17 17:34   ` Dan Williams
  2020-03-17  6:15 ` [PATCH v1 2/2] async_tx: fix possible negative array indexing Rayagonda Kokatanur
  1 sibling, 1 reply; 7+ messages in thread
From: Rayagonda Kokatanur @ 2020-03-17  6:15 UTC (permalink / raw)
  To: Dan Williams, Herbert Xu, David S . Miller, Allison Randal,
	Kate Stewart, Thomas Gleixner, Greg Kroah-Hartman, linux-crypto,
	linux-kernel
  Cc: Rayagonda Kokatanur

Return error upon failure instead of using BUG_ON().
BUG_ON() will crash the kernel.

Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
---
 crypto/async_tx/async_raid6_recov.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/crypto/async_tx/async_raid6_recov.c b/crypto/async_tx/async_raid6_recov.c
index f249142ceac4..33f2a8f8c9f4 100644
--- a/crypto/async_tx/async_raid6_recov.c
+++ b/crypto/async_tx/async_raid6_recov.c
@@ -205,7 +205,9 @@ __2data_recov_5(int disks, size_t bytes, int faila, int failb,
 		good = i;
 		good_srcs++;
 	}
-	BUG_ON(good_srcs > 1);
+
+	if (good_srcs > 1)
+		return NULL;
 
 	p = blocks[disks-2];
 	q = blocks[disks-1];
@@ -339,7 +341,9 @@ async_raid6_2data_recov(int disks, size_t bytes, int faila, int failb,
 	void *scribble = submit->scribble;
 	int non_zero_srcs, i;
 
-	BUG_ON(faila == failb);
+	if (faila == failb)
+		return NULL;
+
 	if (failb < faila)
 		swap(faila, failb);
 
@@ -455,7 +459,8 @@ async_raid6_datap_recov(int disks, size_t bytes, int faila,
 				break;
 		}
 	}
-	BUG_ON(good_srcs == 0);
+	if (good_srcs == 0)
+		return NULL;
 
 	p = blocks[disks-2];
 	q = blocks[disks-1];
-- 
2.17.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v1 2/2] async_tx: fix possible negative array indexing
  2020-03-17  6:15 [PATCH v1 0/2] Remove BUG_ON() and fix -ve array indexing Rayagonda Kokatanur
  2020-03-17  6:15 ` [PATCH v1 1/2] async_tx: return error instead of BUG_ON Rayagonda Kokatanur
@ 2020-03-17  6:15 ` Rayagonda Kokatanur
  2020-03-17 17:36   ` Dan Williams
  1 sibling, 1 reply; 7+ messages in thread
From: Rayagonda Kokatanur @ 2020-03-17  6:15 UTC (permalink / raw)
  To: Dan Williams, Herbert Xu, David S . Miller, Allison Randal,
	Kate Stewart, Thomas Gleixner, Greg Kroah-Hartman, linux-crypto,
	linux-kernel
  Cc: Rayagonda Kokatanur

Fix possible negative array index read in __2data_recov_5() function.

Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
---
 crypto/async_tx/async_raid6_recov.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crypto/async_tx/async_raid6_recov.c b/crypto/async_tx/async_raid6_recov.c
index 33f2a8f8c9f4..9cd016cb2d09 100644
--- a/crypto/async_tx/async_raid6_recov.c
+++ b/crypto/async_tx/async_raid6_recov.c
@@ -206,7 +206,7 @@ __2data_recov_5(int disks, size_t bytes, int faila, int failb,
 		good_srcs++;
 	}
 
-	if (good_srcs > 1)
+	if ((good_srcs > 1) || (good < 0))
 		return NULL;
 
 	p = blocks[disks-2];
-- 
2.17.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1 1/2] async_tx: return error instead of BUG_ON
  2020-03-17  6:15 ` [PATCH v1 1/2] async_tx: return error instead of BUG_ON Rayagonda Kokatanur
@ 2020-03-17 17:34   ` Dan Williams
  0 siblings, 0 replies; 7+ messages in thread
From: Dan Williams @ 2020-03-17 17:34 UTC (permalink / raw)
  To: Rayagonda Kokatanur
  Cc: Herbert Xu, David S . Miller, Allison Randal, Kate Stewart,
	Thomas Gleixner, Greg Kroah-Hartman, linux-crypto,
	Linux Kernel Mailing List

On Mon, Mar 16, 2020 at 11:16 PM Rayagonda Kokatanur
<rayagonda.kokatanur@broadcom.com> wrote:
>
> Return error upon failure instead of using BUG_ON().
> BUG_ON() will crash the kernel.
>
> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>

I don't think this patch is worth it, it has 2 problems:

- These conversions are buggy. Upper layers assume that the calls will
fall back to synchronous operation internally if they return NULL.

- These assertions indicate a major programming error that should not
be silently ignored. They need to be WARN_ON_ONCE() at a minimum, but
only if the above issue is solved.

These assertions are validating the raid implementation in raid5.c
which has been correctly handling this path for several years. The
risk of the code reorganization to "fix" this is higher than the
benefit given zero reports of these actually triggering in production.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1 2/2] async_tx: fix possible negative array indexing
  2020-03-17  6:15 ` [PATCH v1 2/2] async_tx: fix possible negative array indexing Rayagonda Kokatanur
@ 2020-03-17 17:36   ` Dan Williams
  2020-03-18  7:17     ` Rayagonda Kokatanur
  0 siblings, 1 reply; 7+ messages in thread
From: Dan Williams @ 2020-03-17 17:36 UTC (permalink / raw)
  To: Rayagonda Kokatanur
  Cc: Herbert Xu, David S . Miller, Allison Randal, Kate Stewart,
	Thomas Gleixner, Greg Kroah-Hartman, linux-crypto,
	Linux Kernel Mailing List

On Mon, Mar 16, 2020 at 11:16 PM Rayagonda Kokatanur
<rayagonda.kokatanur@broadcom.com> wrote:
>
> Fix possible negative array index read in __2data_recov_5() function.
>
> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
> ---
>  crypto/async_tx/async_raid6_recov.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/crypto/async_tx/async_raid6_recov.c b/crypto/async_tx/async_raid6_recov.c
> index 33f2a8f8c9f4..9cd016cb2d09 100644
> --- a/crypto/async_tx/async_raid6_recov.c
> +++ b/crypto/async_tx/async_raid6_recov.c
> @@ -206,7 +206,7 @@ __2data_recov_5(int disks, size_t bytes, int faila, int failb,
>                 good_srcs++;
>         }
>
> -       if (good_srcs > 1)
> +       if ((good_srcs > 1) || (good < 0))
>                 return NULL;

Read the code again, I don't see how this can happen.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1 2/2] async_tx: fix possible negative array indexing
  2020-03-17 17:36   ` Dan Williams
@ 2020-03-18  7:17     ` Rayagonda Kokatanur
  2020-03-18 15:53       ` Dan Williams
  0 siblings, 1 reply; 7+ messages in thread
From: Rayagonda Kokatanur @ 2020-03-18  7:17 UTC (permalink / raw)
  To: Dan Williams
  Cc: Herbert Xu, David S . Miller, Allison Randal, Kate Stewart,
	Thomas Gleixner, Greg Kroah-Hartman, linux-crypto,
	Linux Kernel Mailing List

On Tue, Mar 17, 2020 at 11:06 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> On Mon, Mar 16, 2020 at 11:16 PM Rayagonda Kokatanur
> <rayagonda.kokatanur@broadcom.com> wrote:
> >
> > Fix possible negative array index read in __2data_recov_5() function.
> >
> > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
> > ---
> >  crypto/async_tx/async_raid6_recov.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/crypto/async_tx/async_raid6_recov.c b/crypto/async_tx/async_raid6_recov.c
> > index 33f2a8f8c9f4..9cd016cb2d09 100644
> > --- a/crypto/async_tx/async_raid6_recov.c
> > +++ b/crypto/async_tx/async_raid6_recov.c
> > @@ -206,7 +206,7 @@ __2data_recov_5(int disks, size_t bytes, int faila, int failb,
> >                 good_srcs++;
> >         }
> >
> > -       if (good_srcs > 1)
> > +       if ((good_srcs > 1) || (good < 0))
> >                 return NULL;
>
> Read the code again, I don't see how this can happen.

This case can happen and it is reported by coverity tool.
In the for loop , the condition "if (blocks[i] == NULL)" true all the
time then variable 'good' will be -1.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v1 2/2] async_tx: fix possible negative array indexing
  2020-03-18  7:17     ` Rayagonda Kokatanur
@ 2020-03-18 15:53       ` Dan Williams
  0 siblings, 0 replies; 7+ messages in thread
From: Dan Williams @ 2020-03-18 15:53 UTC (permalink / raw)
  To: Rayagonda Kokatanur
  Cc: Herbert Xu, David S . Miller, Allison Randal, Kate Stewart,
	Thomas Gleixner, Greg Kroah-Hartman, linux-crypto,
	Linux Kernel Mailing List

On Wed, Mar 18, 2020 at 12:17 AM Rayagonda Kokatanur
<rayagonda.kokatanur@broadcom.com> wrote:
>
> On Tue, Mar 17, 2020 at 11:06 PM Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > On Mon, Mar 16, 2020 at 11:16 PM Rayagonda Kokatanur
> > <rayagonda.kokatanur@broadcom.com> wrote:
> > >
> > > Fix possible negative array index read in __2data_recov_5() function.
> > >
> > > Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
> > > ---
> > >  crypto/async_tx/async_raid6_recov.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/crypto/async_tx/async_raid6_recov.c b/crypto/async_tx/async_raid6_recov.c
> > > index 33f2a8f8c9f4..9cd016cb2d09 100644
> > > --- a/crypto/async_tx/async_raid6_recov.c
> > > +++ b/crypto/async_tx/async_raid6_recov.c
> > > @@ -206,7 +206,7 @@ __2data_recov_5(int disks, size_t bytes, int faila, int failb,
> > >                 good_srcs++;
> > >         }
> > >
> > > -       if (good_srcs > 1)
> > > +       if ((good_srcs > 1) || (good < 0))
> > >                 return NULL;
> >
> > Read the code again, I don't see how this can happen.
>
> This case can happen and it is reported by coverity tool.
> In the for loop , the condition "if (blocks[i] == NULL)" true all the
> time then variable 'good' will be -1.

Just because coverity reports it does not make it true. Please go read
the full call path that gets us to this call and identify how all
entries in blocks[] could be NULL. Hint, the answer is in how
async_raid6_2data_recov() calls __2data_recov_5().

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-03-18 15:53 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-17  6:15 [PATCH v1 0/2] Remove BUG_ON() and fix -ve array indexing Rayagonda Kokatanur
2020-03-17  6:15 ` [PATCH v1 1/2] async_tx: return error instead of BUG_ON Rayagonda Kokatanur
2020-03-17 17:34   ` Dan Williams
2020-03-17  6:15 ` [PATCH v1 2/2] async_tx: fix possible negative array indexing Rayagonda Kokatanur
2020-03-17 17:36   ` Dan Williams
2020-03-18  7:17     ` Rayagonda Kokatanur
2020-03-18 15:53       ` Dan Williams

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).