lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <Z9gE1B__mP6F0b9N@pathway.suse.cz>
Date: Mon, 17 Mar 2025 12:17:40 +0100
From: Petr Mladek <pmladek@...e.com>
To: Joe Lawrence <joe.lawrence@...hat.com>
Cc: Nicolai Stange <nstange@...e.de>, live-patching@...r.kernel.org,
	linux-kernel@...r.kernel.org, Josh Poimboeuf <jpoimboe@...nel.org>,
	Miroslav Benes <mbenes@...e.cz>
Subject: Re: [PATCH v1 18/19] Documentation/livepatch: Update documentation
 for state, callbacks, and shadow variables

Hi,

I am sorry for the late reply. I have read the mail on Friday and then
forgot to come back to it last Monday...

On Fri 2025-03-07 10:50:42, Joe Lawrence wrote:
> On 3/7/25 07:26, Petr Mladek wrote:
> > On Thu 2025-03-06 17:54:41, Joe Lawrence wrote:
> >> Finally, the patchset adds .is_shadow and .callbacks.  A short sequence
> >> of livepatches may look like:
> >>
> >>   klp_patch A               |  klp_patch B
> >>     .states[x]              |    .states[y]
> >>       .id            = 42   |      .id            = 42
> >>       .callbacks            |      .callbacks
> >>       .block_disable        |      .block_disable
> >>       .is_shadow            |      .is_shadow
> >>
> >> is there any harm or confusion if the two patches' state 42 contained
> >> disparate .callbacks, .block_disable, or .is_shadow contents?
> > 
> > Yes, two incompatible states with the same .id would break things.
> > The callbacks won't be called and the old shadow variables
> > won't get freed during an atomic replace.
> > 
> > It is responsibility of the author of the livepatches to use
> > different .id for different states.
> > 
> > I am not sure if we could prevent mistakes. Hmm, we might add
> > a check that every .id is there only once in the patch.states[] array.
> > Also we could add a human readable .name of the state and ensure
> > that it is the same. Or something like this.
> > 
> 
> Well, providing the same state twice in the same klp_patch seems highly
> likely a bug by livepatch author.  That's worth a WARN?

Yes, I agree. I'll add the check and warning in the next revision of
the patch set.


> I'm not sure what to think about the same state id provided by two
> klp_patches.  For a atomic-replace series of patches, if the state
> content is the same, it's effectively like handing off cleanup
> responsibility for that state to the incoming patch, right?

Exactly. And I could imagine an usage of the same state even without
the atomic replace. For example, more livepatches could share the same shadow
variable. Or they might need the same semantic change of a data
structure which would require updating the data by the state callbacks.


> If the state content changes, that would mean that the incoming patch is
> redefining the state... which could be ok?

Using the same state .id for different purpose is _not_ ok.

We could also imagine the state as a reference count of its users.
The pre_patch/post_patch callbacks are called when it is introduced
(refcount goes from 0 -> 1). And the pre_unpatch/post_unpatch
callbacks are called when the state is being removed (refcount
drops from 1 -> 0). [*]

This won't work when two different states share the same .id.
The callbacks won't be called when the 2nd one is added
or when the 1st one is removed.

That said, I do not know how to check that two states have different
semantic when the atomic replace is _not_ used. We could prohibit it.
But I think that there are valid use-cases, especially when
using cumulative livepatches. So, I would keep it allowed.

[*] Note that the current code does not count to refcount number.
    It just checks whether the state is used in other enabled livepatches,
    see is_state_in_other_patches().


Best Regards,
Petr

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ