Re: [PATCH 2/4] livepatch: Handle failing allocation of shadow variables in the selftest
On Wed, 30 Jan 2019, Petr Mladek wrote:
> On Mon 2019-01-21 13:14:38, Miroslav Benes wrote:
> > Hi,
> >
> > On Wed, 16 Jan 2019, Petr Mladek wrote:
> >
> > > Do not dereference pointers to the shadow variables when either
> > > klp_shadow_alloc() or klp_shadow_get() fail.
> >
> > I may misunderstand the patch, so bear with me, please. Is this because of
> > a possible null pointer dereference? If yes, shouldn't this say rather
> > "when both klp_shadow_alloc() and klp_shadow_get() fail"?
>
> Well, klp_shadow_get() could fail also from other reasons if there is
> a bug in the implementation.
Yes, but I meant that if only klp_shadow_alloc() or klp_shadow_get()
failed, it would be caught by ret == sv1 comparison and you would not need
to add checking of ret at the beginning.
> > > There is no need to check the other locations explicitly. The test
> > > would fail if any allocation fails. And the existing messages, printed
> > > during the test, provide enough information to debug eventual problems.
>
> Heh, this is actually the reason why I did not add the check
> for shadow_alloc(). Any error would be detected later
> with klp_shadow_get() calls that should get tested anyway.
>
> Hmm, when I think about it. A good practice is to handle
> klp_shadow_allow() or klp_shadow_get() failures immediately.
> After all, it is the sample code that people might follow.
I think so.
> > > Signed-off-by: Petr Mladek
> > > ---
> > > lib/livepatch/test_klp_shadow_vars.c | 8
> > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/lib/livepatch/test_klp_shadow_vars.c
> > > b/lib/livepatch/test_klp_shadow_vars.c
> > > index 02f892f941dc..55e6820430dc 100644
> > > --- a/lib/livepatch/test_klp_shadow_vars.c
> > > +++ b/lib/livepatch/test_klp_shadow_vars.c
> > > @@ -162,15 +162,15 @@ static int test_klp_shadow_vars_init(void)
> > >* to expected data.
> > >*/
> > > ret = shadow_get(obj, id);
> > > - if (ret == sv1 && *sv1 == )
> > > + if (ret && ret == sv1 && *sv1 == )
> > > pr_info(" got expected PTR%d -> PTR%d result\n",
> > > ptr_id(sv1), ptr_id(*sv1));
> > > ret = shadow_get(obj + 1, id);
> > > - if (ret == sv2 && *sv2 == )
> > > + if (ret && ret == sv2 && *sv2 == )
> > > pr_info(" got expected PTR%d -> PTR%d result\n",
> > > ptr_id(sv2), ptr_id(*sv2));
> > > ret = shadow_get(obj, id + 1);
> > > - if (ret == sv3 && *sv3 == )
> > > + if (ret && ret == sv3 && *sv3 == )
> > > pr_info(" got expected PTR%d -> PTR%d result\n",
> > > ptr_id(sv3), ptr_id(*sv3));
> >
> > There is one more similar site calling shadow_get(obj, id + 1) which is
> > fixed.
>
> Heh, I think that I did not add the check there on purpose.
> If we are here, shadow_get(obj, id + 1) must have already succeeded
> above.
Yes, but if it failed, you would not notice. The message would not be
printed and that's all. So it is possible to run into the same problematic
error condition here.
> But it is a bad practice. We should always check the output.
> I'll do so in v2.
Agreed.
Miroslav
Re: [PATCH 2/4] livepatch: Handle failing allocation of shadow variables in the selftest
On Mon 2019-01-21 17:40:12, Joe Lawrence wrote: > On Wed, Jan 16, 2019 at 05:17:18PM +0100, Petr Mladek wrote: > > Do not dereference pointers to the shadow variables when either > > klp_shadow_alloc() or klp_shadow_get() fail. > > > > There is no need to check the other locations explicitly. The test > > would fail if any allocation fails. And the existing messages, printed > > during the test, provide enough information to debug eventual problems. > > > > I didn't run the test under those failing conditions, but at looking at > the code, I think it would simply skip the "expected found" > and the test script would complain about not seeing that msg. Accessing an invalid pointer would crash the kernel. > Would it be easier to just bite the bullet and verify sv[0-4] at their > allocation sites? Then later uses (ie, the sv3 dereference that > Miroslav spotted at the bottom) or new code wouldn't fall through the > cracks. As I wrote in the replay to Miroslav. The best practice is to handle errors everywhere. I am going to do so in v2. People might use it as a sample... Best Regards, Petr
Re: [PATCH 2/4] livepatch: Handle failing allocation of shadow variables in the selftest
On Mon 2019-01-21 13:14:38, Miroslav Benes wrote:
> Hi,
>
> On Wed, 16 Jan 2019, Petr Mladek wrote:
>
> > Do not dereference pointers to the shadow variables when either
> > klp_shadow_alloc() or klp_shadow_get() fail.
>
> I may misunderstand the patch, so bear with me, please. Is this because of
> a possible null pointer dereference? If yes, shouldn't this say rather
> "when both klp_shadow_alloc() and klp_shadow_get() fail"?
Well, klp_shadow_get() could fail also from other reasons if there is
a bug in the implementation.
> > There is no need to check the other locations explicitly. The test
> > would fail if any allocation fails. And the existing messages, printed
> > during the test, provide enough information to debug eventual problems.
Heh, this is actually the reason why I did not add the check
for shadow_alloc(). Any error would be detected later
with klp_shadow_get() calls that should get tested anyway.
Hmm, when I think about it. A good practice is to handle
klp_shadow_allow() or klp_shadow_get() failures immediately.
After all, it is the sample code that people might follow.
> > Signed-off-by: Petr Mladek
> > ---
> > lib/livepatch/test_klp_shadow_vars.c | 8
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/livepatch/test_klp_shadow_vars.c
> > b/lib/livepatch/test_klp_shadow_vars.c
> > index 02f892f941dc..55e6820430dc 100644
> > --- a/lib/livepatch/test_klp_shadow_vars.c
> > +++ b/lib/livepatch/test_klp_shadow_vars.c
> > @@ -162,15 +162,15 @@ static int test_klp_shadow_vars_init(void)
> > * to expected data.
> > */
> > ret = shadow_get(obj, id);
> > - if (ret == sv1 && *sv1 == )
> > + if (ret && ret == sv1 && *sv1 == )
> > pr_info(" got expected PTR%d -> PTR%d result\n",
> > ptr_id(sv1), ptr_id(*sv1));
> > ret = shadow_get(obj + 1, id);
> > - if (ret == sv2 && *sv2 == )
> > + if (ret && ret == sv2 && *sv2 == )
> > pr_info(" got expected PTR%d -> PTR%d result\n",
> > ptr_id(sv2), ptr_id(*sv2));
> > ret = shadow_get(obj, id + 1);
> > - if (ret == sv3 && *sv3 == )
> > + if (ret && ret == sv3 && *sv3 == )
> > pr_info(" got expected PTR%d -> PTR%d result\n",
> > ptr_id(sv3), ptr_id(*sv3));
>
> There is one more similar site calling shadow_get(obj, id + 1) which is
> fixed.
Heh, I think that I did not add the check there on purpose.
If we are here, shadow_get(obj, id + 1) must have already succeeded
above.
But it is a bad practice. We should always check the output.
I'll do so in v2.
Best Regards,
Petr
Re: [PATCH 2/4] livepatch: Handle failing allocation of shadow variables in the selftest
On Wed, Jan 16, 2019 at 05:17:18PM +0100, Petr Mladek wrote: > Do not dereference pointers to the shadow variables when either > klp_shadow_alloc() or klp_shadow_get() fail. > > There is no need to check the other locations explicitly. The test > would fail if any allocation fails. And the existing messages, printed > during the test, provide enough information to debug eventual problems. > I didn't run the test under those failing conditions, but at looking at the code, I think it would simply skip the "expected found" and the test script would complain about not seeing that msg. Would it be easier to just bite the bullet and verify sv[0-4] at their allocation sites? Then later uses (ie, the sv3 dereference that Miroslav spotted at the bottom) or new code wouldn't fall through the cracks. -- Joe
Re: [PATCH 2/4] livepatch: Handle failing allocation of shadow variables in the selftest
Hi,
On Wed, 16 Jan 2019, Petr Mladek wrote:
> Do not dereference pointers to the shadow variables when either
> klp_shadow_alloc() or klp_shadow_get() fail.
I may misunderstand the patch, so bear with me, please. Is this because of
a possible null pointer dereference? If yes, shouldn't this say rather
"when both klp_shadow_alloc() and klp_shadow_get() fail"?
> There is no need to check the other locations explicitly. The test
> would fail if any allocation fails. And the existing messages, printed
> during the test, provide enough information to debug eventual problems.
>
> Signed-off-by: Petr Mladek
> ---
> lib/livepatch/test_klp_shadow_vars.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/lib/livepatch/test_klp_shadow_vars.c
> b/lib/livepatch/test_klp_shadow_vars.c
> index 02f892f941dc..55e6820430dc 100644
> --- a/lib/livepatch/test_klp_shadow_vars.c
> +++ b/lib/livepatch/test_klp_shadow_vars.c
> @@ -162,15 +162,15 @@ static int test_klp_shadow_vars_init(void)
>* to expected data.
>*/
> ret = shadow_get(obj, id);
> - if (ret == sv1 && *sv1 == )
> + if (ret && ret == sv1 && *sv1 == )
> pr_info(" got expected PTR%d -> PTR%d result\n",
> ptr_id(sv1), ptr_id(*sv1));
> ret = shadow_get(obj + 1, id);
> - if (ret == sv2 && *sv2 == )
> + if (ret && ret == sv2 && *sv2 == )
> pr_info(" got expected PTR%d -> PTR%d result\n",
> ptr_id(sv2), ptr_id(*sv2));
> ret = shadow_get(obj, id + 1);
> - if (ret == sv3 && *sv3 == )
> + if (ret && ret == sv3 && *sv3 == )
> pr_info(" got expected PTR%d -> PTR%d result\n",
> ptr_id(sv3), ptr_id(*sv3));
There is one more similar site calling shadow_get(obj, id + 1) which is
fixed.
Thanks,
Miroslav
