Re: [PATCH v6] modules: introduce the MODULE_SCMVERSION config

2021-02-02 Thread Will McVicker
Hi Christoph,

Did you see Saravana's response to you in version 5? I'd love to hear
your thoughts.

Thanks,
Will

On Mon, Feb 1, 2021 at 11:42 PM Christoph Hellwig  wrote:
>
> Just to deposit my opposition again:  I don't think this hack for your
> out of tree builds has any place in the kernel tree.


Re: [PATCH v6] modules: introduce the MODULE_SCMVERSION config

2021-02-01 Thread Will McVicker
Hi Jessica and Masahiro,

Could I get a final look at the latest patchset please? All the issues
have been addressed thus far.

Thanks,
Will

On Thu, Jan 21, 2021 at 1:36 PM Will McVicker  wrote:
>
> Config MODULE_SCMVERSION introduces a new module attribute --
> `scmversion` -- which can be used to identify a given module's SCM
> version.  This is very useful for developers that update their kernel
> independently from their kernel modules or vice-versa since the SCM
> version provided by UTS_RELEASE (`uname -r`) will now differ from the
> module's vermagic attribute.
>
> For example, we have a CI setup that tests new kernel changes on the
> hikey960 and db845c devices without updating their kernel modules. When
> these tests fail, we need to be able to identify the exact device
> configuration the test was using. By including MODULE_SCMVERSION, we can
> identify the exact kernel and modules' SCM versions for debugging the
> failures.
>
> Additionally, by exposing the SCM version via the sysfs node
> /sys/module/MODULENAME/scmversion, one can also verify the SCM versions
> of the modules loaded from the initramfs. Currently, modinfo can only
> retrieve module attributes from the module's ko on disk and not from the
> actual module that is loaded in RAM.
>
> You can retrieve the SCM version in two ways,
>
> 1) By using modinfo:
> > modinfo -F scmversion MODULENAME
> 2) By module sysfs node:
> > cat /sys/module/MODULENAME/scmversion
>
> Signed-off-by: Will McVicker 
> ---
> Changelog since v5:
> - Simplified scripts/Makefile.modpost to not check for a relative M= path
> - Added space between -v and $(module_scmversion)
> - Updated modpost.c to not check for a missing argument to -v
>
>  Documentation/ABI/stable/sysfs-module | 18 ++
>  include/linux/module.h|  1 +
>  init/Kconfig  | 14 ++
>  kernel/module.c   |  2 ++
>  scripts/Makefile.modpost  | 14 ++
>  scripts/mod/modpost.c | 22 +-
>  6 files changed, 70 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/ABI/stable/sysfs-module 
> b/Documentation/ABI/stable/sysfs-module
> index 6272ae5fb366..a75d137e79f4 100644
> --- a/Documentation/ABI/stable/sysfs-module
> +++ b/Documentation/ABI/stable/sysfs-module
> @@ -32,3 +32,21 @@ Description:
> Note: If the module is built into the kernel, or if the
> CONFIG_MODULE_UNLOAD kernel configuration value is not 
> enabled,
> this file will not be present.
> +
> +What:  /sys/module/MODULENAME/scmversion
> +Date:  November 2020
> +KernelVersion: 5.12
> +Contact:   Will McVicker 
> +Description:   This read-only file will appear if modpost was supplied with 
> an
> +   SCM version for the module. It can be enabled with the config
> +   MODULE_SCMVERSION. The SCM version is retrieved by
> +   scripts/setlocalversion, which means that the presence of this
> +   file depends on CONFIG_LOCALVERSION_AUTO=y. When read, the SCM
> +   version that the module was compiled with is returned. The SCM
> +   version is returned in the following format::
> +
> +   ===
> +   Git:g[a-f0-9]\+(-dirty)\?
> +   Mercurial:  hg[a-f0-9]\+(-dirty)\?
> +   Subversion: svn[0-9]\+
> +   ===
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 7a0bcb5b1ffc..3b1612193cf9 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -372,6 +372,7 @@ struct module {
> struct module_attribute *modinfo_attrs;
> const char *version;
> const char *srcversion;
> +   const char *scmversion;
> struct kobject *holders_dir;
>
> /* Exported symbols */
> diff --git a/init/Kconfig b/init/Kconfig
> index b77c60f8b963..3d9dac3c4e8f 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -2131,6 +2131,20 @@ config MODULE_SRCVERSION_ALL
>   the version).  With this option, such a "srcversion" field
>   will be created for all modules.  If unsure, say N.
>
> +config MODULE_SCMVERSION
> +   bool "SCM version for modules"
> +   depends on LOCALVERSION_AUTO
> +   help
> + This enables the module attribute "scmversion" which can be used
> + by developers to identify the SCM version of a given module, e.g.
> + git sha1 or hg sha1. The SCM version can be queried by modinfo or
> + via the sysfs node: /sys/modules/M

Re: [PATCH v6] modules: introduce the MODULE_SCMVERSION config

2021-02-08 Thread Will McVicker
Thanks Jessica for all the reviews. I guess we can let this die here
and I'll carry it downstream. At least it's all here for others to
see.

Thanks,
Will

On Wed, Feb 3, 2021 at 7:54 AM Greg Kroah-Hartman
 wrote:
>
> On Wed, Feb 03, 2021 at 04:46:16PM +0100, Jessica Yu wrote:
> > +++ Will McVicker [21/01/21 21:36 +]:
> > > Config MODULE_SCMVERSION introduces a new module attribute --
> > > `scmversion` -- which can be used to identify a given module's SCM
> > > version.  This is very useful for developers that update their kernel
> > > independently from their kernel modules or vice-versa since the SCM
> > > version provided by UTS_RELEASE (`uname -r`) will now differ from the
> > > module's vermagic attribute.
> > >
> > > For example, we have a CI setup that tests new kernel changes on the
> > > hikey960 and db845c devices without updating their kernel modules. When
> > > these tests fail, we need to be able to identify the exact device
> > > configuration the test was using. By including MODULE_SCMVERSION, we can
> > > identify the exact kernel and modules' SCM versions for debugging the
> > > failures.
> > >
> > > Additionally, by exposing the SCM version via the sysfs node
> > > /sys/module/MODULENAME/scmversion, one can also verify the SCM versions
> > > of the modules loaded from the initramfs. Currently, modinfo can only
> > > retrieve module attributes from the module's ko on disk and not from the
> > > actual module that is loaded in RAM.
> > >
> > > You can retrieve the SCM version in two ways,
> > >
> > > 1) By using modinfo:
> > >> modinfo -F scmversion MODULENAME
> > > 2) By module sysfs node:
> > >> cat /sys/module/MODULENAME/scmversion
> >
> > Hi Will,
> >
> > First off, thanks for being patient and being responsive throughout the 
> > patch
> > review process.
> >
> > Personally, I am rather neutral towards this feature. We already provide
> > CONFIG_MODULE_SRCVERSION to provide a checksum of a module's source files 
> > and I
> > think the SCMVERSION is a nicer alternative. I can see how an optional
> > scmversion field might be helpful information for distro developers in 
> > testing
> > environments and in situations where it is possible for the kernel and 
> > modules
> > to be updated/packaged separately (for instance, the kernel selftest modules
> > under lib/ are in-tree modules that are provided as a separate kernel module
> > package in SLE).
> >
> > Generally, out of principle, I do not want to merge a patch that's been 
> > NAK'd
> > repeatedly; even if I take the patch it'd likely be contested all the way 
> > up to
> > the merge window. So this boils down to whether Christoph (and maybe Greg) 
> > are
> > fine with this being a debug option that's not enabled by default.
>
> I am neutral on this, I don't care one way or the other.
>
> thanks,
>
> greg k-h


Re: [PATCH v1] HID: make arrays usage and value to be the same

2020-12-07 Thread Will McVicker
On Sat, Dec 05, 2020 at 09:59:57AM +0100, Greg KH wrote:
> On Sat, Dec 05, 2020 at 12:48:48AM +0000, Will McVicker wrote:
> > The HID subsystem allows an "HID report field" to have a different
> > number of "values" and "usages" when it is allocated. When a field
> > struct is created, the size of the usage array is guaranteed to be at
> > least as large as the values array, but it may be larger. This leads to
> > a potential out-of-bounds write in
> > __hidinput_change_resolution_multipliers() and an out-of-bounds read in
> > hidinput_count_leds().
> > 
> > To fix this, let's make sure that both the usage and value arrays are
> > the same size.
> > 
> > Signed-off-by: Will McVicker 
> 
> Any reason not to also add a cc: stable on this?
No reason not to include stable. CC'd here.

> 
> And, has this always been the case, or was this caused by some specific
> commit in the past?  If so, a "Fixes:" tag is always nice to included.
I dug into the history and it's been like this for the past 10 years. So yeah
pretty much always like this.

> 
> And finally, as you have a fix for this already, no need to cc:
> security@k.o as there's nothing the people there can do about it now :)
Is that short for secur...@kernel.org? If yes, then I did include them. If no,
do you mind explaining?

> 
> thanks,
> 
> greg k-h


Re: [PATCH v1] HID: make arrays usage and value to be the same

2020-12-07 Thread Will McVicker
On Mon, Dec 07, 2020 at 07:24:16PM +0100, Greg KH wrote:
> On Mon, Dec 07, 2020 at 09:55:48AM -0800, Will McVicker wrote:
> > On Sat, Dec 05, 2020 at 09:59:57AM +0100, Greg KH wrote:
> > > On Sat, Dec 05, 2020 at 12:48:48AM +0000, Will McVicker wrote:
> > > > The HID subsystem allows an "HID report field" to have a different
> > > > number of "values" and "usages" when it is allocated. When a field
> > > > struct is created, the size of the usage array is guaranteed to be at
> > > > least as large as the values array, but it may be larger. This leads to
> > > > a potential out-of-bounds write in
> > > > __hidinput_change_resolution_multipliers() and an out-of-bounds read in
> > > > hidinput_count_leds().
> > > > 
> > > > To fix this, let's make sure that both the usage and value arrays are
> > > > the same size.
> > > > 
> > > > Signed-off-by: Will McVicker 
> > > 
> > > Any reason not to also add a cc: stable on this?
> > No reason not to include stable. CC'd here.
> > 
> > > 
> > > And, has this always been the case, or was this caused by some specific
> > > commit in the past?  If so, a "Fixes:" tag is always nice to included.
> > I dug into the history and it's been like this for the past 10 years. So 
> > yeah
> > pretty much always like this.
> > 
> > > 
> > > And finally, as you have a fix for this already, no need to cc:
> > > security@k.o as there's nothing the people there can do about it now :)
> > Is that short for secur...@kernel.org? If yes, then I did include them. If 
> > no,
> > do you mind explaining?
> 
> Yes, I see you included it, my point was that once you have a patch,
> there is no need to include this email address as all we do at this
> address is work to match up a problem with a developer that can create a
> fix.  You already did this, so no need for us to get involved at all! :)
> 
> thanks,
> 
> greg k-h
Ah okay, thanks for the explanation!

--Will


[PATCH v3 1/2] scripts/setlocalversion: allow running in a subdir

2020-12-08 Thread Will McVicker
Getting the scmversion using scripts/setlocalversion currently only
works when run at the root of a git or mecurial project. This was
introduced in commit 8558f59edf93 ("setlocalversion: Ignote SCMs above
the linux source tree") so that if one is building within a subdir of
a git tree that isn't the kernel git project, then the vermagic wouldn't
include that git sha1. However, the proper solution to that is to just
set this config in your defconfig:

  # CONFIG_LOCALVERSION_AUTO is not set

which is already the default in many defconfigs:

  $ grep -r "CONFIG_LOCALVERSION_AUTO is not set" arch/* | wc -l
  89

So let's bring back this functionality so that we can use
scripts/setlocalversion to capture the SCM version of external modules
that reside within subdirectories of an SCM project.

Signed-off-by: Will McVicker 
---
 scripts/setlocalversion | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/scripts/setlocalversion b/scripts/setlocalversion
index bb709eda96cd..cd42009e675b 100755
--- a/scripts/setlocalversion
+++ b/scripts/setlocalversion
@@ -44,8 +44,7 @@ scm_version()
fi
 
# Check for git and a git repo.
-   if test -z "$(git rev-parse --show-cdup 2>/dev/null)" &&
-  head=$(git rev-parse --verify HEAD 2>/dev/null); then
+   if head=$(git rev-parse --verify HEAD 2>/dev/null); then
 
# If we are at a tagged commit (like "v2.6.30-rc6"), we ignore
# it, because this version is defined in the top level Makefile.
@@ -102,7 +101,7 @@ scm_version()
fi
 
# Check for mercurial and a mercurial repo.
-   if test -d .hg && hgid=$(hg id 2>/dev/null); then
+   if hgid=$(hg id 2>/dev/null); then
# Do we have an tagged version?  If so, latesttagdistance == 1
if [ "$(hg log -r . --template '{latesttagdistance}')" = "1" ]; 
then
id=$(hg log -r . --template '{latesttag}')
-- 
2.29.2.576.ga3fc446d84-goog



[PATCH v3 0/2] modules: add scmversion field

2020-12-08 Thread Will McVicker
Hi All,

Thanks Jessica for the feedback! I have updated the commit message to
include the justification and common use cases for the patch series. I
have also added the config MODULE_SCMVERSION so that this is not enabled
by default.

Please take a look and let me know of any concerns or issues found.

Thanks,
Will

Will McVicker (2):
  scripts/setlocalversion: allow running in a subdir
  modules: introduce the MODULE_SCMVERSION config

 Documentation/ABI/stable/sysfs-module | 18 ++
 include/linux/module.h|  1 +
 init/Kconfig  | 12 
 kernel/module.c   |  2 ++
 scripts/Makefile.modpost  | 22 ++
 scripts/mod/modpost.c | 24 +++-
 scripts/setlocalversion   |  5 ++---
 7 files changed, 80 insertions(+), 4 deletions(-)

-- 
2.29.2.576.ga3fc446d84-goog



[PATCH v3 2/2] modules: introduce the MODULE_SCMVERSION config

2020-12-08 Thread Will McVicker
Config MODULE_SCMVERSION introduces a new module attribute --
`scmversion` -- which can be used to identify a given module's SCM
version.  This is very useful for developers that update their kernel
independently from their kernel modules or vice-versa since the SCM
version provided by UTS_RELEASE (`uname -r`) will now differ from the
module's vermagic attribute.

For example, we have a CI setup that tests new kernel changes on the
hikey960 and db845c devices without updating their kernel modules. When
these tests fail, we need to be able to identify the exact device
configuration the test was using. By including MODULE_SCMVERSION, we can
identify the exact kernel and modules' SCM versions for debugging the
failures.

Additionally, by exposing the SCM version via the sysfs node
/sys/module/MODULENAME/scmversion, one can also verify the SCM versions
of the modules loaded from the initramfs. Currently, modinfo can only
retrieve module attributes from the module's ko on disk and not from the
actual module that is loaded in RAM.

You can retrieve the SCM version in two ways,

1) By using modinfo:
> modinfo -F scmversion MODULENAME
2) By module sysfs node:
> cat /sys/module/MODULENAME/scmversion

Signed-off-by: Will McVicker 
---
 Documentation/ABI/stable/sysfs-module | 18 ++
 include/linux/module.h|  1 +
 init/Kconfig  | 12 
 kernel/module.c   |  2 ++
 scripts/Makefile.modpost  | 22 ++
 scripts/mod/modpost.c | 24 +++-
 6 files changed, 78 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/stable/sysfs-module 
b/Documentation/ABI/stable/sysfs-module
index 6272ae5fb366..2ba731767737 100644
--- a/Documentation/ABI/stable/sysfs-module
+++ b/Documentation/ABI/stable/sysfs-module
@@ -32,3 +32,21 @@ Description:
Note: If the module is built into the kernel, or if the
CONFIG_MODULE_UNLOAD kernel configuration value is not enabled,
this file will not be present.
+
+What:  /sys/module/MODULENAME/scmversion
+Date:  November 2020
+KernelVersion: 5.11
+Contact:   Will McVicker 
+Description:   This read-only file will appear if modpost was supplied with an
+   SCM version for the module. It can be enabled with the config
+   MODULE_SCMVERSION. The SCM version is retrieved by
+   scripts/setlocalversion, which means that the presence of this
+   file depends on CONFIG_LOCALVERSION_AUTO=y or LOCALVERSION=.
+   When read, the SCM version that the module was compiled with is
+   returned. The SCM version is returned in the following format::
+
+   ===
+   Git:g[a-f0-9]\+(-dirty)\?
+   Mercurial:  hg[a-f0-9]\+(-dirty)\?
+   Subversion: svn[0-9]\+
+   ===
diff --git a/include/linux/module.h b/include/linux/module.h
index 6264617bab4d..63137ca5147b 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -372,6 +372,7 @@ struct module {
struct module_attribute *modinfo_attrs;
const char *version;
const char *srcversion;
+   const char *scmversion;
struct kobject *holders_dir;
 
/* Exported symbols */
diff --git a/init/Kconfig b/init/Kconfig
index 0872a5a2e759..7fc4be1ac62c 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -2131,6 +2131,18 @@ config MODULE_SRCVERSION_ALL
  the version).  With this option, such a "srcversion" field
  will be created for all modules.  If unsure, say N.
 
+config MODULE_SCMVERSION
+   bool "SCM version for modules"
+   depends on LOCALVERSION_AUTO
+   help
+ This enables the module attribute "scmversion" which can be used
+ by developers to identify the SCM version of a given module, e.g.
+ git sha1 or hg sha1. The SCM version can be queried by modinfo or
+ via the sysfs node: /sys/modules/MODULENAME/scmversion. This is
+ useful when the kernel or kernel modules are updated separately
+ since that causes the vermagic of the kernel and the module to
+ differ.
+
 config MODULE_SIG
bool "Module signature verification"
select MODULE_SIG_FORMAT
diff --git a/kernel/module.c b/kernel/module.c
index a4fa44a652a7..a203dab4a03b 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -807,6 +807,7 @@ static struct module_attribute modinfo_##field = {  
  \
 
 MODINFO_ATTR(version);
 MODINFO_ATTR(srcversion);
+MODINFO_ATTR(scmversion);
 
 static char last_unloaded_module[MODULE_NAME_LEN+1];
 
@@ -1269,6 +1270,7 @@ static struct module_attribute *modinfo_attrs[] = {
&module_uevent,
&modinfo_version,
&modinfo_srcversion,
+   &modinfo_scmversion,

Re: [PATCH v5] modules: introduce the MODULE_SCMVERSION config

2021-01-19 Thread Will McVicker
Thanks for the review Masahiro! I'll upload v6 shortly.

--Will

On Sun, Jan 17, 2021 at 7:21 AM Masahiro Yamada  wrote:
>
> On Fri, Jan 8, 2021 at 9:30 AM Will McVicker  wrote:
> >
> > Config MODULE_SCMVERSION introduces a new module attribute --
> > `scmversion` -- which can be used to identify a given module's SCM
> > version.  This is very useful for developers that update their kernel
> > independently from their kernel modules or vice-versa since the SCM
> > version provided by UTS_RELEASE (`uname -r`) will now differ from the
> > module's vermagic attribute.
> >
> > For example, we have a CI setup that tests new kernel changes on the
> > hikey960 and db845c devices without updating their kernel modules. When
> > these tests fail, we need to be able to identify the exact device
> > configuration the test was using. By including MODULE_SCMVERSION, we can
> > identify the exact kernel and modules' SCM versions for debugging the
> > failures.
> >
> > Additionally, by exposing the SCM version via the sysfs node
> > /sys/module/MODULENAME/scmversion, one can also verify the SCM versions
> > of the modules loaded from the initramfs. Currently, modinfo can only
> > retrieve module attributes from the module's ko on disk and not from the
> > actual module that is loaded in RAM.
> >
> > You can retrieve the SCM version in two ways,
> >
> > 1) By using modinfo:
> > > modinfo -F scmversion MODULENAME
> > 2) By module sysfs node:
> > > cat /sys/module/MODULENAME/scmversion
> >
> > Signed-off-by: Will McVicker 
> > ---
> >  Documentation/ABI/stable/sysfs-module | 18 ++
> >  include/linux/module.h|  1 +
> >  init/Kconfig  | 14 ++
> >  kernel/module.c   |  2 ++
> >  scripts/Makefile.modpost  | 22 ++
> >  scripts/mod/modpost.c | 24 +++-
> >  6 files changed, 80 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/ABI/stable/sysfs-module 
> > b/Documentation/ABI/stable/sysfs-module
> > index 6272ae5fb366..a75d137e79f4 100644
> > --- a/Documentation/ABI/stable/sysfs-module
> > +++ b/Documentation/ABI/stable/sysfs-module
> > @@ -32,3 +32,21 @@ Description:
> > Note: If the module is built into the kernel, or if the
> > CONFIG_MODULE_UNLOAD kernel configuration value is not 
> > enabled,
> > this file will not be present.
> > +
> > +What:  /sys/module/MODULENAME/scmversion
> > +Date:  November 2020
> > +KernelVersion: 5.12
> > +Contact:   Will McVicker 
> > +Description:   This read-only file will appear if modpost was supplied 
> > with an
> > +   SCM version for the module. It can be enabled with the 
> > config
> > +   MODULE_SCMVERSION. The SCM version is retrieved by
> > +   scripts/setlocalversion, which means that the presence of 
> > this
> > +   file depends on CONFIG_LOCALVERSION_AUTO=y. When read, the 
> > SCM
> > +   version that the module was compiled with is returned. The 
> > SCM
> > +   version is returned in the following format::
> > +
> > +   ===
> > +   Git:g[a-f0-9]\+(-dirty)\?
> > +   Mercurial:  hg[a-f0-9]\+(-dirty)\?
> > +   Subversion: svn[0-9]\+
> > +   ===
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index 7a0bcb5b1ffc..3b1612193cf9 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -372,6 +372,7 @@ struct module {
> > struct module_attribute *modinfo_attrs;
> > const char *version;
> > const char *srcversion;
> > +   const char *scmversion;
> > struct kobject *holders_dir;
> >
> > /* Exported symbols */
> > diff --git a/init/Kconfig b/init/Kconfig
> > index b77c60f8b963..3d9dac3c4e8f 100644
> > --- a/init/Kconfig
> > +++ b/init/Kconfig
> > @@ -2131,6 +2131,20 @@ config MODULE_SRCVERSION_ALL
> >   the version).  With this option, such a "srcversion" field
> >   will be created for all modules.  If unsure, say N.
> >
> > +config MODULE_SCMVERSION
> > +   bool "SCM version for modules"
> > +   depends on LOCALVERSION_AUTO
> > +   help
> > + This enables t

Re: [PATCH v5] modules: introduce the MODULE_SCMVERSION config

2021-01-19 Thread Will McVicker
Hi Masahiro,

After testing your suggestions for module_srcpath, I remembered why I
needed this:

+ifneq ($(realpath $(srctree)/$(KBUILD_EXTMOD) 2>/dev/null),)
+   module_srcpath := $(srctree)/$(KBUILD_EXTMOD)
+else
+   module_srcpath := $(KBUILD_EXTMOD)
+endif

Basically KBUILD_EXTMOD actually has two uses when defined as a relative path:

(1) defines the path to the external module's source relative to $(srctree)
(2) defines the path to the external module's build artifacts relative
to $(objtree)

Since setlocalversion is run from $(objtree), we need to use
$(srctree)/$(KBUILD_EXTMOD) when it's a relative path. Just to note,
if I do just use KBUILD_EXTMOD, then the script setlocalversion fails
to find the srctree and returns an empty string.

Please correct me if I'm wrong. I messed around with this by adding
many prints to Makefile.modpost and setlocalversion to make sure
everything was defined as expected.

Thanks,
Will

On Tue, Jan 19, 2021 at 1:48 PM Will McVicker  wrote:
>
> Thanks for the review Masahiro! I'll upload v6 shortly.
>
> --Will
>
> On Sun, Jan 17, 2021 at 7:21 AM Masahiro Yamada  wrote:
> >
> > On Fri, Jan 8, 2021 at 9:30 AM Will McVicker  
> > wrote:
> > >
> > > Config MODULE_SCMVERSION introduces a new module attribute --
> > > `scmversion` -- which can be used to identify a given module's SCM
> > > version.  This is very useful for developers that update their kernel
> > > independently from their kernel modules or vice-versa since the SCM
> > > version provided by UTS_RELEASE (`uname -r`) will now differ from the
> > > module's vermagic attribute.
> > >
> > > For example, we have a CI setup that tests new kernel changes on the
> > > hikey960 and db845c devices without updating their kernel modules. When
> > > these tests fail, we need to be able to identify the exact device
> > > configuration the test was using. By including MODULE_SCMVERSION, we can
> > > identify the exact kernel and modules' SCM versions for debugging the
> > > failures.
> > >
> > > Additionally, by exposing the SCM version via the sysfs node
> > > /sys/module/MODULENAME/scmversion, one can also verify the SCM versions
> > > of the modules loaded from the initramfs. Currently, modinfo can only
> > > retrieve module attributes from the module's ko on disk and not from the
> > > actual module that is loaded in RAM.
> > >
> > > You can retrieve the SCM version in two ways,
> > >
> > > 1) By using modinfo:
> > > > modinfo -F scmversion MODULENAME
> > > 2) By module sysfs node:
> > > > cat /sys/module/MODULENAME/scmversion
> > >
> > > Signed-off-by: Will McVicker 
> > > ---
> > >  Documentation/ABI/stable/sysfs-module | 18 ++
> > >  include/linux/module.h|  1 +
> > >  init/Kconfig  | 14 ++
> > >  kernel/module.c   |  2 ++
> > >  scripts/Makefile.modpost  | 22 ++
> > >  scripts/mod/modpost.c | 24 +++-
> > >  6 files changed, 80 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/ABI/stable/sysfs-module 
> > > b/Documentation/ABI/stable/sysfs-module
> > > index 6272ae5fb366..a75d137e79f4 100644
> > > --- a/Documentation/ABI/stable/sysfs-module
> > > +++ b/Documentation/ABI/stable/sysfs-module
> > > @@ -32,3 +32,21 @@ Description:
> > > Note: If the module is built into the kernel, or if the
> > > CONFIG_MODULE_UNLOAD kernel configuration value is not 
> > > enabled,
> > > this file will not be present.
> > > +
> > > +What:  /sys/module/MODULENAME/scmversion
> > > +Date:  November 2020
> > > +KernelVersion: 5.12
> > > +Contact:   Will McVicker 
> > > +Description:   This read-only file will appear if modpost was supplied 
> > > with an
> > > +   SCM version for the module. It can be enabled with the 
> > > config
> > > +   MODULE_SCMVERSION. The SCM version is retrieved by
> > > +   scripts/setlocalversion, which means that the presence of 
> > > this
> > > +   file depends on CONFIG_LOCALVERSION_AUTO=y. When read, 
> > > the SCM
> > > +   version that the module was compiled with is returned. 
> > > The SCM
> > > +   version is 

Re: [PATCH v4] modules: introduce the MODULE_SCMVERSION config

2021-01-07 Thread Will McVicker
Hi Jessica,

I looked into the LOCALVERSION= part more and finally figured out how
that part of the script works. I wasn't familiar with how the
parameter substitution worked -- ${parameter:+alt_value}. I verified
that we will only get an SCM version returned when LOCALVERSION_AUTO=y
and updated the Documentation in v5.

Thanks,
Will

On Thu, Jan 7, 2021 at 6:31 AM Jessica Yu  wrote:
>
> +++ Will McVicker [06/01/21 10:44 -0800]:
> >Thanks for the vacation notice Jessica! I'm just letting you know I'm
> >back as well and am happy to respond to any concerns regarding v4 when
> >you get all caught up.
> >
> >I hope you had a relaxing holiday :)
>
> Hi Will - thanks, same to you!
>
> >On Fri, Dec 18, 2020 at 4:01 AM Jessica Yu  wrote:
> >>
> >> +++ Will McVicker [16/12/20 22:08 +]:
> >> >Config MODULE_SCMVERSION introduces a new module attribute --
> >> >`scmversion` -- which can be used to identify a given module's SCM
> >> >version.  This is very useful for developers that update their kernel
> >> >independently from their kernel modules or vice-versa since the SCM
> >> >version provided by UTS_RELEASE (`uname -r`) will now differ from the
> >> >module's vermagic attribute.
> >> >
> >> >For example, we have a CI setup that tests new kernel changes on the
> >> >hikey960 and db845c devices without updating their kernel modules. When
> >> >these tests fail, we need to be able to identify the exact device
> >> >configuration the test was using. By including MODULE_SCMVERSION, we can
> >> >identify the exact kernel and modules' SCM versions for debugging the
> >> >failures.
> >> >
> >> >Additionally, by exposing the SCM version via the sysfs node
> >> >/sys/module/MODULENAME/scmversion, one can also verify the SCM versions
> >> >of the modules loaded from the initramfs. Currently, modinfo can only
> >> >retrieve module attributes from the module's ko on disk and not from the
> >> >actual module that is loaded in RAM.
> >> >
> >> >You can retrieve the SCM version in two ways,
> >> >
> >> >1) By using modinfo:
> >> >> modinfo -F scmversion MODULENAME
> >> >2) By module sysfs node:
> >> >> cat /sys/module/MODULENAME/scmversion
> >> >
> >> >Signed-off-by: Will McVicker 
> >> >---
> [ added back diff ]
> >> >Changelog since v3:
> >> >- Dropped [PATCH v2 1/2] scripts/setlocalversion: allow running in a 
> >> >subdir
> >> >
> >> > Documentation/ABI/stable/sysfs-module | 18 ++
> >> > include/linux/module.h|  1 +
> >> > init/Kconfig  | 12 
> >> > kernel/module.c   |  2 ++
> >> > scripts/Makefile.modpost  | 22 ++
> >> > scripts/mod/modpost.c | 24 +++-
> >> > 6 files changed, 78 insertions(+), 1 deletion(-)
> >> >
> >> >diff --git a/Documentation/ABI/stable/sysfs-module 
> >> >b/Documentation/ABI/stable/sysfs-module
> >> >index 6272ae5fb366..2ba731767737 100644
> >> >--- a/Documentation/ABI/stable/sysfs-module
> >> >+++ b/Documentation/ABI/stable/sysfs-module
> >> >@@ -32,3 +32,21 @@ Description:
> >> >Note: If the module is built into the kernel, or if the
> >> >CONFIG_MODULE_UNLOAD kernel configuration value is not 
> >> > enabled,
> >> >this file will not be present.
> >> >+
> >> >+What:  /sys/module/MODULENAME/scmversion
> >> >+Date:  November 2020
> >> >+KernelVersion: 5.11
>
> I guess we'll have to bump KernelVersion now (sorry about the timing!)
>
> >> >+Contact:   Will McVicker 
> >> >+Description:   This read-only file will appear if modpost was 
> >> >supplied with an
> >> >+   SCM version for the module. It can be enabled with the config
> >> >+   MODULE_SCMVERSION. The SCM version is retrieved by
> >> >+   scripts/setlocalversion, which means that the presence of this
> >> >+   file depends on CONFIG_LOCALVERSION_AUTO=y or LOCALVERSION=.
>
> I think the "or LOCALVERSION=" part is inaccurate, right? We need
> just LOCALVERSION_AUTO for the full scm string for this to work.

[PATCH v5] modules: introduce the MODULE_SCMVERSION config

2021-01-07 Thread Will McVicker
Config MODULE_SCMVERSION introduces a new module attribute --
`scmversion` -- which can be used to identify a given module's SCM
version.  This is very useful for developers that update their kernel
independently from their kernel modules or vice-versa since the SCM
version provided by UTS_RELEASE (`uname -r`) will now differ from the
module's vermagic attribute.

For example, we have a CI setup that tests new kernel changes on the
hikey960 and db845c devices without updating their kernel modules. When
these tests fail, we need to be able to identify the exact device
configuration the test was using. By including MODULE_SCMVERSION, we can
identify the exact kernel and modules' SCM versions for debugging the
failures.

Additionally, by exposing the SCM version via the sysfs node
/sys/module/MODULENAME/scmversion, one can also verify the SCM versions
of the modules loaded from the initramfs. Currently, modinfo can only
retrieve module attributes from the module's ko on disk and not from the
actual module that is loaded in RAM.

You can retrieve the SCM version in two ways,

1) By using modinfo:
> modinfo -F scmversion MODULENAME
2) By module sysfs node:
> cat /sys/module/MODULENAME/scmversion

Signed-off-by: Will McVicker 
---
 Documentation/ABI/stable/sysfs-module | 18 ++
 include/linux/module.h|  1 +
 init/Kconfig  | 14 ++
 kernel/module.c   |  2 ++
 scripts/Makefile.modpost  | 22 ++
 scripts/mod/modpost.c | 24 +++-
 6 files changed, 80 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/stable/sysfs-module 
b/Documentation/ABI/stable/sysfs-module
index 6272ae5fb366..a75d137e79f4 100644
--- a/Documentation/ABI/stable/sysfs-module
+++ b/Documentation/ABI/stable/sysfs-module
@@ -32,3 +32,21 @@ Description:
Note: If the module is built into the kernel, or if the
CONFIG_MODULE_UNLOAD kernel configuration value is not enabled,
this file will not be present.
+
+What:  /sys/module/MODULENAME/scmversion
+Date:  November 2020
+KernelVersion: 5.12
+Contact:   Will McVicker 
+Description:   This read-only file will appear if modpost was supplied with an
+   SCM version for the module. It can be enabled with the config
+   MODULE_SCMVERSION. The SCM version is retrieved by
+   scripts/setlocalversion, which means that the presence of this
+   file depends on CONFIG_LOCALVERSION_AUTO=y. When read, the SCM
+   version that the module was compiled with is returned. The SCM
+   version is returned in the following format::
+
+   ===
+   Git:g[a-f0-9]\+(-dirty)\?
+   Mercurial:  hg[a-f0-9]\+(-dirty)\?
+   Subversion: svn[0-9]\+
+   ===
diff --git a/include/linux/module.h b/include/linux/module.h
index 7a0bcb5b1ffc..3b1612193cf9 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -372,6 +372,7 @@ struct module {
struct module_attribute *modinfo_attrs;
const char *version;
const char *srcversion;
+   const char *scmversion;
struct kobject *holders_dir;
 
/* Exported symbols */
diff --git a/init/Kconfig b/init/Kconfig
index b77c60f8b963..3d9dac3c4e8f 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -2131,6 +2131,20 @@ config MODULE_SRCVERSION_ALL
  the version).  With this option, such a "srcversion" field
  will be created for all modules.  If unsure, say N.
 
+config MODULE_SCMVERSION
+   bool "SCM version for modules"
+   depends on LOCALVERSION_AUTO
+   help
+ This enables the module attribute "scmversion" which can be used
+ by developers to identify the SCM version of a given module, e.g.
+ git sha1 or hg sha1. The SCM version can be queried by modinfo or
+ via the sysfs node: /sys/modules/MODULENAME/scmversion. This is
+ useful when the kernel or kernel modules are updated separately
+ since that causes the vermagic of the kernel and the module to
+ differ.
+
+ If unsure, say N.
+
 config MODULE_SIG
bool "Module signature verification"
select MODULE_SIG_FORMAT
diff --git a/kernel/module.c b/kernel/module.c
index 4bf30e4b3eaa..d0b359c7e9c9 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -837,6 +837,7 @@ static struct module_attribute modinfo_##field = {  
  \
 
 MODINFO_ATTR(version);
 MODINFO_ATTR(srcversion);
+MODINFO_ATTR(scmversion);
 
 static char last_unloaded_module[MODULE_NAME_LEN+1];
 
@@ -1298,6 +1299,7 @@ static struct module_attribute *modinfo_attrs[] = {
&module_uevent,
&modinfo_version,
&modinfo_srcversion,
+   

Re: [PATCH v1] HID: make arrays usage and value to be the same

2021-01-14 Thread Will McVicker
Hi Jiri,

I noticed this hasn't merged yet. So just sending a friendly reminder.

Thanks,
Will

On Thu, Dec 17, 2020 at 10:42 AM Will McVicker  wrote:
>
> Great! Thanks for the reply.
>
> --Will
>
> On Thu, Dec 17, 2020 at 2:19 AM Jiri Kosina  wrote:
> >
> > On Mon, 14 Dec 2020, Will McVicker wrote:
> >
> > > > The HID subsystem allows an "HID report field" to have a different
> > > > number of "values" and "usages" when it is allocated. When a field
> > > > struct is created, the size of the usage array is guaranteed to be at
> > > > least as large as the values array, but it may be larger. This leads to
> > > > a potential out-of-bounds write in
> > > > __hidinput_change_resolution_multipliers() and an out-of-bounds read in
> > > > hidinput_count_leds().
> > > >
> > > > To fix this, let's make sure that both the usage and value arrays are
> > > > the same size.
> > > >
> > > > Signed-off-by: Will McVicker 
> > > > ---
> > > >  drivers/hid/hid-core.c | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > > > index 56172fe6995c..8a8b2b982f83 100644
> > > > --- a/drivers/hid/hid-core.c
> > > > +++ b/drivers/hid/hid-core.c
> > > > @@ -90,7 +90,7 @@ EXPORT_SYMBOL_GPL(hid_register_report);
> > > >   * Register a new field for this report.
> > > >   */
> > > >
> > > > -static struct hid_field *hid_register_field(struct hid_report *report, 
> > > > unsigned usages, unsigned values)
> > > > +static struct hid_field *hid_register_field(struct hid_report *report, 
> > > > unsigned usages)
> > > >  {
> > > > struct hid_field *field;
> > > >
> > > > @@ -101,7 +101,7 @@ static struct hid_field *hid_register_field(struct 
> > > > hid_report *report, unsigned
> > > >
> > > > field = kzalloc((sizeof(struct hid_field) +
> > > >  usages * sizeof(struct hid_usage) +
> > > > -values * sizeof(unsigned)), GFP_KERNEL);
> > > > +usages * sizeof(unsigned)), GFP_KERNEL);
> > > > if (!field)
> > > > return NULL;
> > > >
> > > > @@ -300,7 +300,7 @@ static int hid_add_field(struct hid_parser *parser, 
> > > > unsigned report_type, unsign
> > > > usages = max_t(unsigned, parser->local.usage_index,
> > > >  parser->global.report_count);
> > > >
> > > > -   field = hid_register_field(report, usages, 
> > > > parser->global.report_count);
> > > > +   field = hid_register_field(report, usages);
> > > > if (!field)
> > > > return 0;
> > > >
> > > > --
> > > > 2.29.2.576.ga3fc446d84-goog
> > > >
> > >
> > > Hi Jiri and Benjamin,
> > >
> > > This is a friendly reminder in case this got lost in your inbox.
> >
> > Hi Will,
> >
> > I am planning to merge it once the merge window is over.
> >
> > --
> > Jiri Kosina
> > SUSE Labs
> >


Re: [PATCH v5] modules: introduce the MODULE_SCMVERSION config

2021-01-14 Thread Will McVicker
Thanks Jessica for the reviews!

--Will

On Tue, Jan 12, 2021 at 7:55 AM Jessica Yu  wrote:
>
> +++ Will McVicker [08/01/21 00:30 +]:
> >Config MODULE_SCMVERSION introduces a new module attribute --
> >`scmversion` -- which can be used to identify a given module's SCM
> >version.  This is very useful for developers that update their kernel
> >independently from their kernel modules or vice-versa since the SCM
> >version provided by UTS_RELEASE (`uname -r`) will now differ from the
> >module's vermagic attribute.
> >
> >For example, we have a CI setup that tests new kernel changes on the
> >hikey960 and db845c devices without updating their kernel modules. When
> >these tests fail, we need to be able to identify the exact device
> >configuration the test was using. By including MODULE_SCMVERSION, we can
> >identify the exact kernel and modules' SCM versions for debugging the
> >failures.
> >
> >Additionally, by exposing the SCM version via the sysfs node
> >/sys/module/MODULENAME/scmversion, one can also verify the SCM versions
> >of the modules loaded from the initramfs. Currently, modinfo can only
> >retrieve module attributes from the module's ko on disk and not from the
> >actual module that is loaded in RAM.
> >
> >You can retrieve the SCM version in two ways,
> >
> >1) By using modinfo:
> >> modinfo -F scmversion MODULENAME
> >2) By module sysfs node:
> >> cat /sys/module/MODULENAME/scmversion
> >
> >Signed-off-by: Will McVicker 
>
> Hi Will,
>
> Thanks for v5, I'm fine with this patch now that we've made it a
> configurable developer/debug option to supply an scmversion field for
> in-tree modules (although, this currently works for out-of-tree
> modules too since we based module_srcpath on KBUILD_EXTMOD in the
> external module case). I basically see this option as an alternative
> to CONFIG_MODULE_SRCVERSION_ALL to aid distro developers debug issues
> when the kernel and in-tree modules are updated separately.
>
> In any case, since there was pushback in our earlier discussions, I'd
> like to ask if there are any remaining objections to this patch.
>
> Masahiro, are you fine with the Makefile and modpost changes?
>
> Thanks,
>
> Jessica
>
> >---
> > Documentation/ABI/stable/sysfs-module | 18 ++
> > include/linux/module.h|  1 +
> > init/Kconfig  | 14 ++
> > kernel/module.c   |  2 ++
> > scripts/Makefile.modpost  | 22 ++
> > scripts/mod/modpost.c | 24 +++-
> > 6 files changed, 80 insertions(+), 1 deletion(-)
> >
> >diff --git a/Documentation/ABI/stable/sysfs-module 
> >b/Documentation/ABI/stable/sysfs-module
> >index 6272ae5fb366..a75d137e79f4 100644
> >--- a/Documentation/ABI/stable/sysfs-module
> >+++ b/Documentation/ABI/stable/sysfs-module
> >@@ -32,3 +32,21 @@ Description:
> >   Note: If the module is built into the kernel, or if the
> >   CONFIG_MODULE_UNLOAD kernel configuration value is not 
> > enabled,
> >   this file will not be present.
> >+
> >+What: /sys/module/MODULENAME/scmversion
> >+Date: November 2020
> >+KernelVersion:5.12
> >+Contact:  Will McVicker 
> >+Description:  This read-only file will appear if modpost was supplied with 
> >an
> >+  SCM version for the module. It can be enabled with the config
> >+  MODULE_SCMVERSION. The SCM version is retrieved by
> >+  scripts/setlocalversion, which means that the presence of this
> >+  file depends on CONFIG_LOCALVERSION_AUTO=y. When read, the SCM
> >+  version that the module was compiled with is returned. The SCM
> >+  version is returned in the following format::
> >+
> >+  ===
> >+  Git:g[a-f0-9]\+(-dirty)\?
> >+  Mercurial:  hg[a-f0-9]\+(-dirty)\?
> >+  Subversion: svn[0-9]\+
> >+  ===
> >diff --git a/include/linux/module.h b/include/linux/module.h
> >index 7a0bcb5b1ffc..3b1612193cf9 100644
> >--- a/include/linux/module.h
> >+++ b/include/linux/module.h
> >@@ -372,6 +372,7 @@ struct module {
> >   struct module_attribute *modinfo_attrs;
> >   const char *version;
> >   const char *srcversion;
> >+  const char *scmversion;
> >   struct kobject *holders_dir;
> >
> >  

[PATCH v1 0/1] gpiolib: Don't free if pin ranges are not defined

2020-11-30 Thread Will McVicker
This patch fixes commit 2ab73c6d8323 ("gpio: Support GPIO controllers
without pin-ranges") where an imbalance of requests and frees to the
pinctrl driver were introduced.

Edmond Chung (1):
  gpiolib: Don't free if pin ranges are not defined

 drivers/gpio/gpiolib.c | 5 +
 1 file changed, 5 insertions(+)

-- 
2.29.2.454.gaff20da3a2-goog



[PATCH v1 1/1] gpiolib: Don't free if pin ranges are not defined

2020-11-30 Thread Will McVicker
From: Edmond Chung 

A similar check was added in gpiochip_generic_request, but not in free.
This has caused an imbalance count of request vs. free calls to the
pinctrl driver. This patch is targeted to fix that issue.

Fixes: 2ab73c6d8323 ("gpio: Support GPIO controllers without pin-ranges")
Signed-off-by: Edmond Chung 
Signed-off-by: Andrew Chant 
Signed-off-by: Will McVicker 
---
 drivers/gpio/gpiolib.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 089ddcaa9bc6..6e3c4d7a7d14 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1806,6 +1806,11 @@ EXPORT_SYMBOL_GPL(gpiochip_generic_request);
  */
 void gpiochip_generic_free(struct gpio_chip *gc, unsigned offset)
 {
+#ifdef CONFIG_PINCTRL
+   if (list_empty(&gc->gpiodev->pin_ranges))
+   return;
+#endif
+
pinctrl_gpio_free(gc->gpiodev->base + offset);
 }
 EXPORT_SYMBOL_GPL(gpiochip_generic_free);
-- 
2.29.2.454.gaff20da3a2-goog



Re: [PATCH v2 0/2] Adds support to capture module's SCM version

2020-12-04 Thread Will McVicker
On Fri, Dec 04, 2020 at 07:51:59AM +, Christoph Hellwig wrote:
> I think your decription still shows absolutely no benefit for the
> kernel, so I'not sure why anyone would want to waste time on this.
Hi Christoph,

Did you get a chance to read my earlier responses regarding the uses for
in-tree modules?

The biggest benefit for the upstream community is being about to get the SCM
version for *any* module (including in-tree modules) in the initramfs via the
sysfs node. Currently there is no way to do that and there is no guarantee that
those modules in the initramfs were compiled with the running kernel. In fact,
running,

  modinfo -F vermagic MODULENAME

will return an invalid vermagic string if the same module with different
vermagic strings exists in the initramfs and on disk because modinfo only looks
at the module on disk (not in memory).

The second most useful benefit goes hand-in-hand with MODVERSIONS. The purpose
of MODVERSIONS is to create a stable interface that allows one to update the
kernel and kernel modules (including in-tree modules) independently. So when
developers do update their kernels independently (think for security bug
fixes), the `scmversion` attribute guarantees developers that they can still
identify the modules' or kernel's SCM version.

I hope that helps. If not, then please let me know why these reasons "show
absolutely no benefit for the kernel?"

Thanks,
Will


Re: [PATCH v2 0/2] Adds support to capture module's SCM version

2020-12-04 Thread Will McVicker
On Fri, Dec 04, 2020 at 06:18:08PM +, Christoph Hellwig wrote:
> On Fri, Dec 04, 2020 at 10:13:56AM -0800, Will McVicker wrote:
> > On Fri, Dec 04, 2020 at 07:51:59AM +, Christoph Hellwig wrote:
> > > I think your decription still shows absolutely no benefit for the
> > > kernel, so I'not sure why anyone would want to waste time on this.
> > Hi Christoph,
> > 
> > Did you get a chance to read my earlier responses regarding the uses for
> > in-tree modules?
> > 
> > The biggest benefit for the upstream community is being about to get the SCM
> > version for *any* module (including in-tree modules) in the initramfs via 
> > the
> > sysfs node. Currently there is no way to do that and there is no guarantee 
> > that
> 
> That assumes the SCM version of a module has any kind of meaning for
> an in-tree module.  Which it doesn't.  If you care about the SCM version
> of an in-tree module the only thing we need is one single global sysfs
> file.
Why doesn't it have meaning? With MODVERSIONS, you are able to update in-tree
kernel modules independently of the kernel. That means you can update as many
in-tree modules as you want which would create many different SCM versions (1
per every module update). Also you can update the kernel independently of the
in-tree modules introducing another SCM version.

--Will


[PATCH v1] HID: make arrays usage and value to be the same

2020-12-04 Thread Will McVicker
The HID subsystem allows an "HID report field" to have a different
number of "values" and "usages" when it is allocated. When a field
struct is created, the size of the usage array is guaranteed to be at
least as large as the values array, but it may be larger. This leads to
a potential out-of-bounds write in
__hidinput_change_resolution_multipliers() and an out-of-bounds read in
hidinput_count_leds().

To fix this, let's make sure that both the usage and value arrays are
the same size.

Signed-off-by: Will McVicker 
---
 drivers/hid/hid-core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 56172fe6995c..8a8b2b982f83 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -90,7 +90,7 @@ EXPORT_SYMBOL_GPL(hid_register_report);
  * Register a new field for this report.
  */
 
-static struct hid_field *hid_register_field(struct hid_report *report, 
unsigned usages, unsigned values)
+static struct hid_field *hid_register_field(struct hid_report *report, 
unsigned usages)
 {
struct hid_field *field;
 
@@ -101,7 +101,7 @@ static struct hid_field *hid_register_field(struct 
hid_report *report, unsigned
 
field = kzalloc((sizeof(struct hid_field) +
 usages * sizeof(struct hid_usage) +
-values * sizeof(unsigned)), GFP_KERNEL);
+usages * sizeof(unsigned)), GFP_KERNEL);
if (!field)
return NULL;
 
@@ -300,7 +300,7 @@ static int hid_add_field(struct hid_parser *parser, 
unsigned report_type, unsign
usages = max_t(unsigned, parser->local.usage_index,
 parser->global.report_count);
 
-   field = hid_register_field(report, usages, parser->global.report_count);
+   field = hid_register_field(report, usages);
if (!field)
return 0;
 
-- 
2.29.2.576.ga3fc446d84-goog



[PATCH v2 0/2] Adds support to capture module's SCM version

2020-11-24 Thread Will McVicker
Hi All,

I have updated the patchset to:

 *) Include Documentation.
 *) Use a consistent output pattern for the SCM version.

In my debugging, I found that the vermagic reported by modinfo can actually
vary based on how the module was loaded. For example, if you have a module in
the initramfs that is newer than the module on disk, then the initramfs module
will be loaded (not the one on disk) during boot. Then, when you run the
command:

  $ modinfo MODULENAME

The vermagic returned will actually be the vermagic of the module on disk and
not the one in the initramfs which was actually loaded. With that being said,
adding this scmversion attribute ensures that you can *always* get the correct
SCM version of the module that loaded.

Please take a look at the updated patch and provide any comments you find.

Thanks,
Will

Will McVicker (2):
  scripts/setlocalversion: allow running in a subdir
  modules: add scmversion field

 Documentation/ABI/stable/sysfs-module | 17 +
 include/linux/module.h|  1 +
 kernel/module.c   |  2 ++
 scripts/Makefile.modpost  | 20 
 scripts/mod/modpost.c | 24 +++-
 scripts/setlocalversion   |  5 ++---
 6 files changed, 65 insertions(+), 4 deletions(-)

-- 
2.29.2.454.gaff20da3a2-goog



[PATCH v2 2/2] modules: add scmversion field

2020-11-24 Thread Will McVicker
Add the modinfo field `scmversion` to include the SCM version of kernel
modules, e.g. git sha1. This allows one to identify the exact source
code version of a given kernel module.

You can retrieve it in two ways,

1) By using modinfo
> modinfo -F scmversion 
2) By module sysfs node
> cat /sys/module//scmversion

Signed-off-by: Will McVicker 
---
 Documentation/ABI/stable/sysfs-module | 17 +
 include/linux/module.h|  1 +
 kernel/module.c   |  2 ++
 scripts/Makefile.modpost  | 20 
 scripts/mod/modpost.c | 24 +++-
 5 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/stable/sysfs-module 
b/Documentation/ABI/stable/sysfs-module
index 6272ae5fb366..46c99ec927ab 100644
--- a/Documentation/ABI/stable/sysfs-module
+++ b/Documentation/ABI/stable/sysfs-module
@@ -32,3 +32,20 @@ Description:
Note: If the module is built into the kernel, or if the
CONFIG_MODULE_UNLOAD kernel configuration value is not enabled,
this file will not be present.
+
+What:  /sys/module/MODULENAME/scmversion
+Date:  November 2020
+KernelVersion: 5.10
+Contact:   Will McVicker 
+Description:   This read-only file will appear if modpost was supplied with an
+   SCM version for the module. The SCM version is retrieved by
+   scripts/setlocalversion, which means that the presence of this
+   file depends on CONFIG_LOCALVERSION_AUTO=y or LOCALVERSION=.
+   When read, the SCM version that the module was compiled with is
+   returned. The SCM version is returned in the following format::
+
+   ===
+   Git:g[a-f0-9]\+(-dirty)\?
+   Mercurial:  hg[a-f0-9]\+(-dirty)\?
+   Subversion: svn[0-9]\+
+   ===
diff --git a/include/linux/module.h b/include/linux/module.h
index 6264617bab4d..63137ca5147b 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -372,6 +372,7 @@ struct module {
struct module_attribute *modinfo_attrs;
const char *version;
const char *srcversion;
+   const char *scmversion;
struct kobject *holders_dir;
 
/* Exported symbols */
diff --git a/kernel/module.c b/kernel/module.c
index a4fa44a652a7..a203dab4a03b 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -807,6 +807,7 @@ static struct module_attribute modinfo_##field = {  
  \
 
 MODINFO_ATTR(version);
 MODINFO_ATTR(srcversion);
+MODINFO_ATTR(scmversion);
 
 static char last_unloaded_module[MODULE_NAME_LEN+1];
 
@@ -1269,6 +1270,7 @@ static struct module_attribute *modinfo_attrs[] = {
&module_uevent,
&modinfo_version,
&modinfo_srcversion,
+   &modinfo_scmversion,
&modinfo_initstate,
&modinfo_coresize,
&modinfo_initsize,
diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index f54b6ac37ac2..fb4ddf2bf794 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -66,6 +66,7 @@ ifeq ($(KBUILD_EXTMOD),)
 
 input-symdump := vmlinux.symvers
 output-symdump := Module.symvers
+module_srcpath := $(srctree)
 
 else
 
@@ -77,6 +78,17 @@ src := $(obj)
 include $(if $(wildcard $(KBUILD_EXTMOD)/Kbuild), \
  $(KBUILD_EXTMOD)/Kbuild, $(KBUILD_EXTMOD)/Makefile)
 
+# Get the external module's source path. KBUILD_EXTMOD could either be an
+# absolute path or relative path from $(srctree). This makes sure that we
+# aren't using a relative path from a separate working directory (O= or
+# KBUILD_OUTPUT) since that may not be the actual module's SCM project path. So
+# check the path relative to $(srctree) first.
+ifneq ($(realpath $(srctree)/$(KBUILD_EXTMOD) 2>/dev/null),)
+   module_srcpath := $(srctree)/$(KBUILD_EXTMOD)
+else
+   module_srcpath := $(KBUILD_EXTMOD)
+endif
+
 # modpost option for external modules
 MODPOST += -e
 
@@ -85,6 +97,14 @@ output-symdump := $(KBUILD_EXTMOD)/Module.symvers
 
 endif
 
+# Get the SCM version of the module. Sed verifies setlocalversion returns
+# a proper revision based on the SCM type, e.g. git, mercurial, or svn.
+module_scmversion := $(shell $(srctree)/scripts/setlocalversion 
$(module_srcpath) | \
+   sed -n 's/.*-\(\(g\|hg\)[a-fA-F0-9]\+\(-dirty\)\?\|svn[0-9]\+\).*/\1/p')
+ifneq ($(module_scmversion),)
+MODPOST += -v$(module_scmversion)
+endif
+
 # modpost options for modules (both in-kernel and external)
 MODPOST += \
$(addprefix -i ,$(wildcard $(input-symdump))) \
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index f882ce0d9327..db71e0c9ab20 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -30,6 +30,8 @@ static int have_vmlinux = 0;
 static int all_versions = 0;
 /* If we are modposting external module set to 1 */
 stat

[PATCH v2 1/2] scripts/setlocalversion: allow running in a subdir

2020-11-24 Thread Will McVicker
Getting the scmversion using scripts/setlocalversion currently only
works when run at the root of a git or mecurial project. This was
introduced in commit 8558f59edf93 ("setlocalversion: Ignote SCMs above
the linux source tree") so that if one is building within a subdir of
a git tree that isn't the kernel git project, then the vermagic wouldn't
include that git sha1. However, the proper solution to that is to just
set this config in your defconfig:

  # CONFIG_LOCALVERSION_AUTO is not set

which is already the default in many defconfigs:

  $ grep -r "CONFIG_LOCALVERSION_AUTO is not set" arch/* | wc -l
  89

So let's bring back this functionality so that we can use
scripts/setlocalversion to capture the SCM version of external modules
that reside within subdirectories of an SCM project.

Signed-off-by: Will McVicker 
---
 scripts/setlocalversion | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/scripts/setlocalversion b/scripts/setlocalversion
index bb709eda96cd..cd42009e675b 100755
--- a/scripts/setlocalversion
+++ b/scripts/setlocalversion
@@ -44,8 +44,7 @@ scm_version()
fi
 
# Check for git and a git repo.
-   if test -z "$(git rev-parse --show-cdup 2>/dev/null)" &&
-  head=$(git rev-parse --verify HEAD 2>/dev/null); then
+   if head=$(git rev-parse --verify HEAD 2>/dev/null); then
 
# If we are at a tagged commit (like "v2.6.30-rc6"), we ignore
# it, because this version is defined in the top level Makefile.
@@ -102,7 +101,7 @@ scm_version()
fi
 
# Check for mercurial and a mercurial repo.
-   if test -d .hg && hgid=$(hg id 2>/dev/null); then
+   if hgid=$(hg id 2>/dev/null); then
# Do we have an tagged version?  If so, latesttagdistance == 1
if [ "$(hg log -r . --template '{latesttagdistance}')" = "1" ]; 
then
id=$(hg log -r . --template '{latesttag}')
-- 
2.29.2.454.gaff20da3a2-goog



Re: [PATCH v1] HID: make arrays usage and value to be the same

2020-12-17 Thread Will McVicker
Great! Thanks for the reply.

--Will

On Thu, Dec 17, 2020 at 2:19 AM Jiri Kosina  wrote:
>
> On Mon, 14 Dec 2020, Will McVicker wrote:
>
> > > The HID subsystem allows an "HID report field" to have a different
> > > number of "values" and "usages" when it is allocated. When a field
> > > struct is created, the size of the usage array is guaranteed to be at
> > > least as large as the values array, but it may be larger. This leads to
> > > a potential out-of-bounds write in
> > > __hidinput_change_resolution_multipliers() and an out-of-bounds read in
> > > hidinput_count_leds().
> > >
> > > To fix this, let's make sure that both the usage and value arrays are
> > > the same size.
> > >
> > > Signed-off-by: Will McVicker 
> > > ---
> > >  drivers/hid/hid-core.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > > index 56172fe6995c..8a8b2b982f83 100644
> > > --- a/drivers/hid/hid-core.c
> > > +++ b/drivers/hid/hid-core.c
> > > @@ -90,7 +90,7 @@ EXPORT_SYMBOL_GPL(hid_register_report);
> > >   * Register a new field for this report.
> > >   */
> > >
> > > -static struct hid_field *hid_register_field(struct hid_report *report, 
> > > unsigned usages, unsigned values)
> > > +static struct hid_field *hid_register_field(struct hid_report *report, 
> > > unsigned usages)
> > >  {
> > > struct hid_field *field;
> > >
> > > @@ -101,7 +101,7 @@ static struct hid_field *hid_register_field(struct 
> > > hid_report *report, unsigned
> > >
> > > field = kzalloc((sizeof(struct hid_field) +
> > >  usages * sizeof(struct hid_usage) +
> > > -values * sizeof(unsigned)), GFP_KERNEL);
> > > +usages * sizeof(unsigned)), GFP_KERNEL);
> > > if (!field)
> > > return NULL;
> > >
> > > @@ -300,7 +300,7 @@ static int hid_add_field(struct hid_parser *parser, 
> > > unsigned report_type, unsign
> > > usages = max_t(unsigned, parser->local.usage_index,
> > >  parser->global.report_count);
> > >
> > > -   field = hid_register_field(report, usages, 
> > > parser->global.report_count);
> > > +   field = hid_register_field(report, usages);
> > > if (!field)
> > > return 0;
> > >
> > > --
> > > 2.29.2.576.ga3fc446d84-goog
> > >
> >
> > Hi Jiri and Benjamin,
> >
> > This is a friendly reminder in case this got lost in your inbox.
>
> Hi Will,
>
> I am planning to merge it once the merge window is over.
>
> --
> Jiri Kosina
> SUSE Labs
>


Re: [PATCH] kbuild: warn a relative path used for M= in out-of-tree builds

2021-01-20 Thread Will McVicker
CC: kernel-t...@android.com

Thanks Masahiro!

I'd like to also note that the Android kernel build system is
leveraging this behaviour as sort of an "O=" for external modules [1].
This way the external module's build artifacts are contained separate
from the kernel build artifacts and the module's source code.

--Will

[1] 
https://android.googlesource.com/kernel/build/+/refs/heads/master/build.sh#748

On Wed, Jan 20, 2021 at 11:31 AM Masahiro Yamada  wrote:
>
> Surprising to me, Kbuild can handle a relative M= path while having
> in-kernel objects in a separate directory. In this case, the M= option
> actually defines:
>
>  - the path to external module sources relative to $(srctree)
>  - the path to external module objects relative to $(objtree)
>
> For example,
>
>   make O=foo/bar M=baz
>
> ... will find the source files in baz, and create objects in foo/bar/baz.
>
> Currently, it works like that because of the presence of VPATH and
> --include-dir=$(abs_srctree), but this behavior is subtle and cannot
> be guaranteed in the future. I do not know if it was even officially
> supported ever.
>
> I do not recommend this usage.
>
> Reported-by: Will McVicker 
> Signed-off-by: Masahiro Yamada 
> ---
>
>  Makefile | 7 +++
>  1 file changed, 7 insertions(+)
>
> diff --git a/Makefile b/Makefile
> index b0e4767735dc..e42a27447eae 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1699,6 +1699,13 @@ else # KBUILD_EXTMOD
>  #  Install the modules built in the module directory
>  #  Assumes install directory is already created
>
> +ifdef building_out_of_srctree
> +ifneq ($(filter-out /%, $(KBUILD_EXTMOD)),)
> +$(warning The combination of out-of-tree build and relative M= path is not 
> well supported.)
> +$(warning Please do not complain about any trouble happening under this 
> circumstance.)
> +endif
> +endif
> +
>  # We are always building only modules.
>  KBUILD_BUILTIN :=
>  KBUILD_MODULES := 1
> --
> 2.27.0
>


[PATCH v6] modules: introduce the MODULE_SCMVERSION config

2021-01-21 Thread Will McVicker
Config MODULE_SCMVERSION introduces a new module attribute --
`scmversion` -- which can be used to identify a given module's SCM
version.  This is very useful for developers that update their kernel
independently from their kernel modules or vice-versa since the SCM
version provided by UTS_RELEASE (`uname -r`) will now differ from the
module's vermagic attribute.

For example, we have a CI setup that tests new kernel changes on the
hikey960 and db845c devices without updating their kernel modules. When
these tests fail, we need to be able to identify the exact device
configuration the test was using. By including MODULE_SCMVERSION, we can
identify the exact kernel and modules' SCM versions for debugging the
failures.

Additionally, by exposing the SCM version via the sysfs node
/sys/module/MODULENAME/scmversion, one can also verify the SCM versions
of the modules loaded from the initramfs. Currently, modinfo can only
retrieve module attributes from the module's ko on disk and not from the
actual module that is loaded in RAM.

You can retrieve the SCM version in two ways,

1) By using modinfo:
> modinfo -F scmversion MODULENAME
2) By module sysfs node:
> cat /sys/module/MODULENAME/scmversion

Signed-off-by: Will McVicker 
---
Changelog since v5:
- Simplified scripts/Makefile.modpost to not check for a relative M= path
- Added space between -v and $(module_scmversion)
- Updated modpost.c to not check for a missing argument to -v

 Documentation/ABI/stable/sysfs-module | 18 ++
 include/linux/module.h|  1 +
 init/Kconfig  | 14 ++
 kernel/module.c   |  2 ++
 scripts/Makefile.modpost  | 14 ++
 scripts/mod/modpost.c | 22 +-
 6 files changed, 70 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/stable/sysfs-module 
b/Documentation/ABI/stable/sysfs-module
index 6272ae5fb366..a75d137e79f4 100644
--- a/Documentation/ABI/stable/sysfs-module
+++ b/Documentation/ABI/stable/sysfs-module
@@ -32,3 +32,21 @@ Description:
Note: If the module is built into the kernel, or if the
CONFIG_MODULE_UNLOAD kernel configuration value is not enabled,
this file will not be present.
+
+What:  /sys/module/MODULENAME/scmversion
+Date:  November 2020
+KernelVersion: 5.12
+Contact:   Will McVicker 
+Description:   This read-only file will appear if modpost was supplied with an
+   SCM version for the module. It can be enabled with the config
+   MODULE_SCMVERSION. The SCM version is retrieved by
+   scripts/setlocalversion, which means that the presence of this
+   file depends on CONFIG_LOCALVERSION_AUTO=y. When read, the SCM
+   version that the module was compiled with is returned. The SCM
+   version is returned in the following format::
+
+   ===
+   Git:g[a-f0-9]\+(-dirty)\?
+   Mercurial:  hg[a-f0-9]\+(-dirty)\?
+   Subversion: svn[0-9]\+
+   ===
diff --git a/include/linux/module.h b/include/linux/module.h
index 7a0bcb5b1ffc..3b1612193cf9 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -372,6 +372,7 @@ struct module {
struct module_attribute *modinfo_attrs;
const char *version;
const char *srcversion;
+   const char *scmversion;
struct kobject *holders_dir;
 
/* Exported symbols */
diff --git a/init/Kconfig b/init/Kconfig
index b77c60f8b963..3d9dac3c4e8f 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -2131,6 +2131,20 @@ config MODULE_SRCVERSION_ALL
  the version).  With this option, such a "srcversion" field
  will be created for all modules.  If unsure, say N.
 
+config MODULE_SCMVERSION
+   bool "SCM version for modules"
+   depends on LOCALVERSION_AUTO
+   help
+ This enables the module attribute "scmversion" which can be used
+ by developers to identify the SCM version of a given module, e.g.
+ git sha1 or hg sha1. The SCM version can be queried by modinfo or
+ via the sysfs node: /sys/modules/MODULENAME/scmversion. This is
+ useful when the kernel or kernel modules are updated separately
+ since that causes the vermagic of the kernel and the module to
+ differ.
+
+ If unsure, say N.
+
 config MODULE_SIG
bool "Module signature verification"
select MODULE_SIG_FORMAT
diff --git a/kernel/module.c b/kernel/module.c
index 4bf30e4b3eaa..d0b359c7e9c9 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -837,6 +837,7 @@ static struct module_attribute modinfo_##field = {  
  \
 
 MODINFO_ATTR(version);
 MODINFO_ATTR(srcversion);
+MODINFO_ATTR(scmversion);
 
 static char last_unloaded_module[MODULE_N

Re: [PATCH v4] modules: introduce the MODULE_SCMVERSION config

2021-01-06 Thread Will McVicker
Thanks for the vacation notice Jessica! I'm just letting you know I'm
back as well and am happy to respond to any concerns regarding v4 when
you get all caught up.

I hope you had a relaxing holiday :)

Thanks,
Will

On Fri, Dec 18, 2020 at 4:01 AM Jessica Yu  wrote:
>
> +++ Will McVicker [16/12/20 22:08 +]:
> >Config MODULE_SCMVERSION introduces a new module attribute --
> >`scmversion` -- which can be used to identify a given module's SCM
> >version.  This is very useful for developers that update their kernel
> >independently from their kernel modules or vice-versa since the SCM
> >version provided by UTS_RELEASE (`uname -r`) will now differ from the
> >module's vermagic attribute.
> >
> >For example, we have a CI setup that tests new kernel changes on the
> >hikey960 and db845c devices without updating their kernel modules. When
> >these tests fail, we need to be able to identify the exact device
> >configuration the test was using. By including MODULE_SCMVERSION, we can
> >identify the exact kernel and modules' SCM versions for debugging the
> >failures.
> >
> >Additionally, by exposing the SCM version via the sysfs node
> >/sys/module/MODULENAME/scmversion, one can also verify the SCM versions
> >of the modules loaded from the initramfs. Currently, modinfo can only
> >retrieve module attributes from the module's ko on disk and not from the
> >actual module that is loaded in RAM.
> >
> >You can retrieve the SCM version in two ways,
> >
> >1) By using modinfo:
> >> modinfo -F scmversion MODULENAME
> >2) By module sysfs node:
> >> cat /sys/module/MODULENAME/scmversion
> >
> >Signed-off-by: Will McVicker 
>
> Hi Will, thanks for v4. I'm on vacation for the next two weeks and
> with the current merge window almost over, I hope it'd be OK with you
> if we revisit this early January. Just wanted to give you a heads up.
>
> Thanks and happy holidays!
>
> Jessica
>


Re: [PATCH v3 1/2] scripts/setlocalversion: allow running in a subdir

2020-12-16 Thread Will McVicker
On Fri, Dec 11, 2020 at 04:33:59PM +0100, Jessica Yu wrote:
> Hi Will,
> 
> +++ Will McVicker [08/12/20 20:05 +]:
> > Getting the scmversion using scripts/setlocalversion currently only
> > works when run at the root of a git or mecurial project. This was
> > introduced in commit 8558f59edf93 ("setlocalversion: Ignote SCMs above
> > the linux source tree") so that if one is building within a subdir of
> > a git tree that isn't the kernel git project, then the vermagic wouldn't
> > include that git sha1. However, the proper solution to that is to just
> > set this config in your defconfig:
> > 
> >  # CONFIG_LOCALVERSION_AUTO is not set
> > 
> > which is already the default in many defconfigs:
> > 
> >  $ grep -r "CONFIG_LOCALVERSION_AUTO is not set" arch/* | wc -l
> >  89
> > 
> > So let's bring back this functionality so that we can use
> > scripts/setlocalversion to capture the SCM version of external modules
> > that reside within subdirectories of an SCM project.
> 
> Hm, this seems to essentially be a revert of commit 8558f59edf93.
> AFAICT from light testing it also reintroduces the issue it was
> originally trying to fix, no?
> 
> From the reporter:
> 
>Dan McGee  writes:
>> Note that when in git, you get the appended "+" sign. If
>> LOCALVERSION_AUTO is set, you will get something like
>> "eee-gb01b08c-dirty" (whereas the copy of the tree in /tmp still
>> returns "eee"). It doesn't matter whether the working tree is dirty or
>> clean.
>>
>> Is there a way to disable this? I'm building from a clean tarball that
>> just happens to be unpacked inside a git repository. One would think
>> setting LOCALVERSION_AUTO to false would do it, but no such luck...
> 
> Correct me if I'm wrong, but what I'm understanding is that the
> original reporter was having trouble with setlocalversion appending
> unwanted strings ("+" or "gXXX-dirty" etc) when building from a
> clean tarball that happens to live inside a git repo. Even if
> LOCALVERSION_AUTO is disabled it still appends the "+" string if the
> SCM above the linux source tree is not at an annotated tag. Since
> setlocalversion is getting confused by the presence of a different scm
> that commit fixed this by confining the checks to the root of the
> (possibly git managed) kernel source tree. Masahiro can probably
> better comment since he maintains scripts/*.
> 
> In any case, this patch isn't of interest to in-tree modules, since we
> can generate the scmversion perfectly fine without it, so I doubt it's
> going to get any support here. Would you be fine with dropping the
> first patch or would that pose issues?
> 
> > Signed-off-by: Will McVicker 
> > ---
> > scripts/setlocalversion | 5 ++---
> > 1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/scripts/setlocalversion b/scripts/setlocalversion
> > index bb709eda96cd..cd42009e675b 100755
> > --- a/scripts/setlocalversion
> > +++ b/scripts/setlocalversion
> > @@ -44,8 +44,7 @@ scm_version()
> > fi
> > 
> > # Check for git and a git repo.
> > -   if test -z "$(git rev-parse --show-cdup 2>/dev/null)" &&
> > -  head=$(git rev-parse --verify HEAD 2>/dev/null); then
> > +   if head=$(git rev-parse --verify HEAD 2>/dev/null); then
> > 
> > # If we are at a tagged commit (like "v2.6.30-rc6"), we ignore
> > # it, because this version is defined in the top level Makefile.
> > @@ -102,7 +101,7 @@ scm_version()
> > fi
> > 
> > # Check for mercurial and a mercurial repo.
> > -   if test -d .hg && hgid=$(hg id 2>/dev/null); then
> > +   if hgid=$(hg id 2>/dev/null); then
> > # Do we have an tagged version?  If so, latesttagdistance == 1
> > if [ "$(hg log -r . --template '{latesttagdistance}')" = "1" ]; 
> > then
> > id=$(hg log -r . --template '{latesttag}')
> > -- 
> > 2.29.2.576.ga3fc446d84-goog
> > 
> 
> -- 
> To unsubscribe from this group and stop receiving emails from it, send an 
> email to kernel-team+unsubscr...@android.com.
> 

Hi Jessica,

I'm okay with dropped this first patch since it does only related to external
modules. I'll upload v4 now with just the bits that related to in-tree modules.

Thanks,
Will


[PATCH v4] modules: introduce the MODULE_SCMVERSION config

2020-12-16 Thread Will McVicker
Config MODULE_SCMVERSION introduces a new module attribute --
`scmversion` -- which can be used to identify a given module's SCM
version.  This is very useful for developers that update their kernel
independently from their kernel modules or vice-versa since the SCM
version provided by UTS_RELEASE (`uname -r`) will now differ from the
module's vermagic attribute.

For example, we have a CI setup that tests new kernel changes on the
hikey960 and db845c devices without updating their kernel modules. When
these tests fail, we need to be able to identify the exact device
configuration the test was using. By including MODULE_SCMVERSION, we can
identify the exact kernel and modules' SCM versions for debugging the
failures.

Additionally, by exposing the SCM version via the sysfs node
/sys/module/MODULENAME/scmversion, one can also verify the SCM versions
of the modules loaded from the initramfs. Currently, modinfo can only
retrieve module attributes from the module's ko on disk and not from the
actual module that is loaded in RAM.

You can retrieve the SCM version in two ways,

1) By using modinfo:
> modinfo -F scmversion MODULENAME
2) By module sysfs node:
> cat /sys/module/MODULENAME/scmversion

Signed-off-by: Will McVicker 
---
Changelog since v3:
- Dropped [PATCH v2 1/2] scripts/setlocalversion: allow running in a subdir

 Documentation/ABI/stable/sysfs-module | 18 ++
 include/linux/module.h|  1 +
 init/Kconfig  | 12 
 kernel/module.c   |  2 ++
 scripts/Makefile.modpost  | 22 ++
 scripts/mod/modpost.c | 24 +++-
 6 files changed, 78 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/stable/sysfs-module 
b/Documentation/ABI/stable/sysfs-module
index 6272ae5fb366..2ba731767737 100644
--- a/Documentation/ABI/stable/sysfs-module
+++ b/Documentation/ABI/stable/sysfs-module
@@ -32,3 +32,21 @@ Description:
Note: If the module is built into the kernel, or if the
CONFIG_MODULE_UNLOAD kernel configuration value is not enabled,
this file will not be present.
+
+What:  /sys/module/MODULENAME/scmversion
+Date:  November 2020
+KernelVersion: 5.11
+Contact:   Will McVicker 
+Description:   This read-only file will appear if modpost was supplied with an
+   SCM version for the module. It can be enabled with the config
+   MODULE_SCMVERSION. The SCM version is retrieved by
+   scripts/setlocalversion, which means that the presence of this
+   file depends on CONFIG_LOCALVERSION_AUTO=y or LOCALVERSION=.
+   When read, the SCM version that the module was compiled with is
+   returned. The SCM version is returned in the following format::
+
+   ===
+   Git:g[a-f0-9]\+(-dirty)\?
+   Mercurial:  hg[a-f0-9]\+(-dirty)\?
+   Subversion: svn[0-9]\+
+   ===
diff --git a/include/linux/module.h b/include/linux/module.h
index c4e7a887f469..6bd710308863 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -372,6 +372,7 @@ struct module {
struct module_attribute *modinfo_attrs;
const char *version;
const char *srcversion;
+   const char *scmversion;
struct kobject *holders_dir;
 
/* Exported symbols */
diff --git a/init/Kconfig b/init/Kconfig
index b77c60f8b963..d9ae12f16ba2 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -2131,6 +2131,18 @@ config MODULE_SRCVERSION_ALL
  the version).  With this option, such a "srcversion" field
  will be created for all modules.  If unsure, say N.
 
+config MODULE_SCMVERSION
+   bool "SCM version for modules"
+   depends on LOCALVERSION_AUTO
+   help
+ This enables the module attribute "scmversion" which can be used
+ by developers to identify the SCM version of a given module, e.g.
+ git sha1 or hg sha1. The SCM version can be queried by modinfo or
+ via the sysfs node: /sys/modules/MODULENAME/scmversion. This is
+ useful when the kernel or kernel modules are updated separately
+ since that causes the vermagic of the kernel and the module to
+ differ.
+
 config MODULE_SIG
bool "Module signature verification"
select MODULE_SIG_FORMAT
diff --git a/kernel/module.c b/kernel/module.c
index c3a9e972d3b2..6967cd3abf9b 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -836,6 +836,7 @@ static struct module_attribute modinfo_##field = {  
  \
 
 MODINFO_ATTR(version);
 MODINFO_ATTR(srcversion);
+MODINFO_ATTR(scmversion);
 
 static char last_unloaded_module[MODULE_NAME_LEN+1];
 
@@ -1298,6 +1299,7 @@ static struct module_attribute *modinfo_attrs[] = {
&am

Re: [PATCH v1] HID: make arrays usage and value to be the same

2020-12-14 Thread Will McVicker
On Sat, Dec 05, 2020 at 12:48:48AM +, Will McVicker wrote:
> The HID subsystem allows an "HID report field" to have a different
> number of "values" and "usages" when it is allocated. When a field
> struct is created, the size of the usage array is guaranteed to be at
> least as large as the values array, but it may be larger. This leads to
> a potential out-of-bounds write in
> __hidinput_change_resolution_multipliers() and an out-of-bounds read in
> hidinput_count_leds().
> 
> To fix this, let's make sure that both the usage and value arrays are
> the same size.
> 
> Signed-off-by: Will McVicker 
> ---
>  drivers/hid/hid-core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 56172fe6995c..8a8b2b982f83 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -90,7 +90,7 @@ EXPORT_SYMBOL_GPL(hid_register_report);
>   * Register a new field for this report.
>   */
>  
> -static struct hid_field *hid_register_field(struct hid_report *report, 
> unsigned usages, unsigned values)
> +static struct hid_field *hid_register_field(struct hid_report *report, 
> unsigned usages)
>  {
>   struct hid_field *field;
>  
> @@ -101,7 +101,7 @@ static struct hid_field *hid_register_field(struct 
> hid_report *report, unsigned
>  
>   field = kzalloc((sizeof(struct hid_field) +
>usages * sizeof(struct hid_usage) +
> -  values * sizeof(unsigned)), GFP_KERNEL);
> +  usages * sizeof(unsigned)), GFP_KERNEL);
>   if (!field)
>   return NULL;
>  
> @@ -300,7 +300,7 @@ static int hid_add_field(struct hid_parser *parser, 
> unsigned report_type, unsign
>   usages = max_t(unsigned, parser->local.usage_index,
>parser->global.report_count);
>  
> - field = hid_register_field(report, usages, parser->global.report_count);
> + field = hid_register_field(report, usages);
>   if (!field)
>   return 0;
>  
> -- 
> 2.29.2.576.ga3fc446d84-goog
> 

Hi Jiri and Benjamin,

This is a friendly reminder in case this got lost in your inbox.

Thanks,
Will



[PATCH v1 1/2] scripts/setlocalversion: allow running in a subdir

2020-11-20 Thread Will McVicker
Getting the scmversion using scripts/setlocalversion currently only
works when run at the root of a git or mecurial project. This was
introduced in commit 8558f59edf93 ("setlocalversion: Ignote SCMs above
the linux source tree") so that if one is building within a subdir of
a git tree that isn't the kernel git project, then the vermagic wouldn't
include that git sha1. However, the proper solution to that is to just
set this config in your defconfig:

  # CONFIG_LOCALVERSION_AUTO is not set

which is already the default in many defconfigs:

  $ grep -r "CONFIG_LOCALVERSION_AUTO is not set" arch/* | wc -l
  89

So let's bring back this functionality so that we can use
scripts/setlocalversion to capture the SCM version of external modules
that reside within subdirectories of an SCM project.

Signed-off-by: Will McVicker 
---
 scripts/setlocalversion | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/scripts/setlocalversion b/scripts/setlocalversion
index bb709eda96cd..cd42009e675b 100755
--- a/scripts/setlocalversion
+++ b/scripts/setlocalversion
@@ -44,8 +44,7 @@ scm_version()
fi
 
# Check for git and a git repo.
-   if test -z "$(git rev-parse --show-cdup 2>/dev/null)" &&
-  head=$(git rev-parse --verify HEAD 2>/dev/null); then
+   if head=$(git rev-parse --verify HEAD 2>/dev/null); then
 
# If we are at a tagged commit (like "v2.6.30-rc6"), we ignore
# it, because this version is defined in the top level Makefile.
@@ -102,7 +101,7 @@ scm_version()
fi
 
# Check for mercurial and a mercurial repo.
-   if test -d .hg && hgid=$(hg id 2>/dev/null); then
+   if hgid=$(hg id 2>/dev/null); then
# Do we have an tagged version?  If so, latesttagdistance == 1
if [ "$(hg log -r . --template '{latesttagdistance}')" = "1" ]; 
then
id=$(hg log -r . --template '{latesttag}')
-- 
2.29.2.454.gaff20da3a2-goog



[PATCH v1 2/2] modules: add scmversion field

2020-11-20 Thread Will McVicker
Add the modinfo field `scmversion` to include the SCM version of kernel
modules, e.g. git sha1. This allows one to identify the exact source
code version of a given kernel module.

You can retrieve it in two ways,

1) By using modinfo
> modinfo -F scmversion 
2) By module sysfs node
> cat /sys/module//scmversion

Signed-off-by: Will McVicker 
---
 include/linux/module.h   |  1 +
 kernel/module.c  | 20 
 scripts/Makefile.modpost | 19 +--
 scripts/mod/modpost.c| 28 +++-
 4 files changed, 65 insertions(+), 3 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 6264617bab4d..63137ca5147b 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -372,6 +372,7 @@ struct module {
struct module_attribute *modinfo_attrs;
const char *version;
const char *srcversion;
+   const char *scmversion;
struct kobject *holders_dir;
 
/* Exported symbols */
diff --git a/kernel/module.c b/kernel/module.c
index a4fa44a652a7..be155ec80083 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -807,6 +807,7 @@ static struct module_attribute modinfo_##field = {  
  \
 
 MODINFO_ATTR(version);
 MODINFO_ATTR(srcversion);
+MODINFO_ATTR(scmversion);
 
 static char last_unloaded_module[MODULE_NAME_LEN+1];
 
@@ -1265,10 +1266,29 @@ static ssize_t show_taint(struct module_attribute 
*mattr,
 static struct module_attribute modinfo_taint =
__ATTR(taint, 0444, show_taint, NULL);
 
+/**
+ * struct modinfo_attrs - Module attributes.
+ * @module_uevent: Used to notify udev of events.
+ * @modinfo_version: Module version.
+ * @modinfo_srcversion: Checksum of module source.
+ * @modinfo_scmversion: SCM version of module source.
+ * @modinfo_initstate: Module init state.
+ * @modinfo_coresize: Module core layout size.
+ * @modinfo_initsize: Module init layout size.
+ * @modinfo_taint: Indicates if the module is tainted.
+ * @modinfo_refcnt: Number of references in the kernel to the module.
+ *
+ * These are the module attributes accessible via the sysfs files
+ * /sys/module//.
+ *
+ * The following subset of attributes can also be accessed via the modinfo tool
+ * as well: version, srcversion, and scmversion.
+ */
 static struct module_attribute *modinfo_attrs[] = {
&module_uevent,
&modinfo_version,
&modinfo_srcversion,
+   &modinfo_scmversion,
&modinfo_initstate,
&modinfo_coresize,
&modinfo_initsize,
diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index f54b6ac37ac2..4486eb72240e 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -77,8 +77,23 @@ src := $(obj)
 include $(if $(wildcard $(KBUILD_EXTMOD)/Kbuild), \
  $(KBUILD_EXTMOD)/Kbuild, $(KBUILD_EXTMOD)/Makefile)
 
-# modpost option for external modules
-MODPOST += -e
+# Get the external module's source path. KBUILD_EXTMOD could either be an
+# absolute path or relative path from $(srctree). This makes sure that we
+# aren't using a relative path from a separate working directory (O= or
+# KBUILD_OUTPUT) since that may not be the actual module's SCM project path. So
+# check the path relative to $(srctree) first.
+ifneq ($(realpath $(srctree)/$(KBUILD_EXTMOD) 2>/dev/null),)
+   module_srcpath := $(srctree)/$(KBUILD_EXTMOD)
+else
+   module_srcpath := $(KBUILD_EXTMOD)
+endif
+
+# Get the SCM version of the external module. Sed verifies setlocalversion
+# returns a proper revision based on the SCM type, e.g. git, mercurial, or svn.
+module_scmversion := $(shell $(srctree)/scripts/setlocalversion 
$(module_srcpath) | \
+   sed -n 
's/.*-\(\(\(g\|hg\)[a-fA-F0-9]\+\|svn[0-9]\+\)\(-dirty\)\?\).*\?/\1/p')
+# modpost option for external modules.
+MODPOST += -e$(module_scmversion)
 
 input-symdump := Module.symvers $(KBUILD_EXTRA_SYMBOLS)
 output-symdump := $(KBUILD_EXTMOD)/Module.symvers
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index f882ce0d9327..db59eb2a880d 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -30,6 +30,8 @@ static int have_vmlinux = 0;
 static int all_versions = 0;
 /* If we are modposting external module set to 1 */
 static int external_module = 0;
+#define MODULE_SCMVERSION_SIZE 64
+static char module_scmversion[MODULE_SCMVERSION_SIZE];
 /* Only warn about unresolved symbols */
 static int warn_unresolved = 0;
 /* How a symbol is exported */
@@ -2272,6 +2274,27 @@ static void add_intree_flag(struct buffer *b, int 
is_intree)
buf_printf(b, "\nMODULE_INFO(intree, \"Y\");\n");
 }
 
+/**
+ * add_scmversion() - Adds the MODULE_INFO macro for the scmversion.
+ * @b: Buffer to append to.
+ * @is_intree: Indicates if the module is in-tree or is an external module.
+ *
+ * This function fills in the module attribute `scmversion` for the kernel
+ * module. This is 

[PATCH v1 0/2] Add support to capture external module's SCM version

2020-11-20 Thread Will McVicker
These two patches add module support to capture an external module's SCM
version as a MODULE_INFO() attribute. This allows users to identity the SCM
version of a given kernel module by using the modinfo tool or on the device
via sysfs:

  cat /sys/module//scmversion

It's important to note that the sysfs node is necessary in order to get the SCM
version of modules that were loaded from the ramdisk in first stage init.
I have updated scripts/setlocalversion to support this for git, mercurial, and
subversion.

Here is the example output I used to test these patches with a simple module
versioned with git, hg, and svn:

  $ modinfo simple_module.ko | egrep 'scmversion|vermagic'
  scmversion: gbf35fd9b6412
  vermagic:   5.10.0-rc4-00110-gd83461f36865 SMP mod_unload

  $ modinfo simple_module.ko | egrep 'scmversion|vermagic'
  scmversion: hge5037af323b9
  vermagic:   5.10.0-rc4-00110-gd83461f36865 SMP mod_unload

  $ modinfo simple_module.ko | egrep 'scmversion|vermagic'
  scmversion: svn1
  vermagic:   5.10.0-rc4-00110-gd83461f36865 SMP mod_unload

Will McVicker (2):
  scripts/setlocalversion: allow running in a subdir
  modules: add scmversion field

 include/linux/module.h   |  1 +
 kernel/module.c  | 20 
 scripts/Makefile.modpost | 19 +--
 scripts/mod/modpost.c| 28 +++-
 scripts/setlocalversion  |  5 ++---
 5 files changed, 67 insertions(+), 6 deletions(-)

-- 
2.29.2.454.gaff20da3a2-goog



[PATCH v3 1/1] netfilter: nat: add a range check for l3/l4 protonum

2020-08-24 Thread Will McVicker
The indexes to the nf_nat_l[34]protos arrays come from userspace. So
check the tuple's family, e.g. l3num, when creating the conntrack in
order to prevent an OOB memory access during setup.  Here is an example
kernel panic on 4.14.180 when userspace passes in an index greater than
NFPROTO_NUMPROTO.

Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
Modules linked in:...
Process poc (pid: 5614, stack limit = 0xa3933121)
CPU: 4 PID: 5614 Comm: poc Tainted: G S  W  O4.14.180-g051355490483
Hardware name: Qualcomm Technologies, Inc. SM8150 V2 PM8150 Google Inc. MSM
task: 2a3dfffe task.stack: a3933121
pc : __cfi_check_fail+0x1c/0x24
lr : __cfi_check_fail+0x1c/0x24
...
Call trace:
__cfi_check_fail+0x1c/0x24
name_to_dev_t+0x0/0x468
nfnetlink_parse_nat_setup+0x234/0x258
ctnetlink_parse_nat_setup+0x4c/0x228
ctnetlink_new_conntrack+0x590/0xc40
nfnetlink_rcv_msg+0x31c/0x4d4
netlink_rcv_skb+0x100/0x184
nfnetlink_rcv+0xf4/0x180
netlink_unicast+0x360/0x770
netlink_sendmsg+0x5a0/0x6a4
___sys_sendmsg+0x314/0x46c
SyS_sendmsg+0xb4/0x108
el0_svc_naked+0x34/0x38

Fixes: c1d10adb4a521 ("[NETFILTER]: Add ctnetlink port for nf_conntrack")
Signed-off-by: Will McVicker 
---
 net/netfilter/nf_conntrack_netlink.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/netfilter/nf_conntrack_netlink.c 
b/net/netfilter/nf_conntrack_netlink.c
index 31fa94064a62..0b89609a6e9d 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -1129,6 +1129,8 @@ ctnetlink_parse_tuple(const struct nlattr * const cda[],
if (!tb[CTA_TUPLE_IP])
return -EINVAL;
 
+   if (l3num != NFPROTO_IPV4 && l3num != NFPROTO_IPV6)
+   return -EOPNOTSUPP;
tuple->src.l3num = l3num;
 
err = ctnetlink_parse_tuple_ip(tb[CTA_TUPLE_IP], tuple);
-- 
2.28.0.297.g1956fa8f8d-goog



[PATCH v3 0/1] netfilter: nat: add a range check for l3/l4 protonum

2020-08-24 Thread Will McVicker
Hi Pablo,

> This patch is much smaller and if you confirm this is address the
> issue, then this is awesome.

Yes, I can confirm the updated patch does fix the kernel panic. I have retested
on the Pixel 4 XL with version 4.14.180. Please see the updated patchset v3.

Thanks,
Will


Will McVicker (1):
  netfilter: nat: add a range check for l3/l4 protonum

 net/netfilter/nf_conntrack_netlink.c | 2 ++
 1 file changed, 2 insertions(+)

-- 
2.28.0.297.g1956fa8f8d-goog



[PATCH] arm64: Fix off-by-one vdso trampoline return value

2020-11-11 Thread Will McVicker
Depending on your host nm version, the generated header
`include/generated/vdso32-offsets.h` may have the bottom bit set for the
thumb vdso offset addresses (as observed when using llvm-nm). This
results in an additional +1 for thumb vdso trampoline return values
since compat_setup_return() already includes `vdso_trampoline + thumb`.
As a result, I see a SIGBUS error when running the LTP test
syscalls.rt_sigaction01. To fix this, let's clear the bottom bit of the
vdso_offset in the VDSO_SYMBOL macro.

Test: LTP test syscalls.rt_sigaction01
Fixes: f01703b3d2e6 ("arm64: compat: Get sigreturn trampolines from vDSO")
Signed-off-by: Will McVicker 
---
 arch/arm64/include/asm/vdso.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/vdso.h b/arch/arm64/include/asm/vdso.h
index f99dcb94b438..a7384379e8e1 100644
--- a/arch/arm64/include/asm/vdso.h
+++ b/arch/arm64/include/asm/vdso.h
@@ -23,7 +23,7 @@
 
 #define VDSO_SYMBOL(base, name)
   \
 ({\
-   (void *)(vdso_offset_##name - VDSO_LBASE + (unsigned long)(base)); \
+   (void *)((vdso_offset_##name & ~1UL) - VDSO_LBASE + (unsigned 
long)(base)); \
 })
 
 #endif /* !__ASSEMBLY__ */
-- 
2.29.2.299.gdc1121823c-goog



[PATCH 0/1] Netfilter OOB memory access security patch

2020-07-27 Thread Will McVicker
Hi,
The attached patch fixes an OOB memory access security bug. The bug is
already fixed in the upstream kernel due to the vulnerable code being
refactored in commit fe2d0020994c ("netfilter: nat: remove
l4proto->in_range") and commit d6c4c8ffb5e5 ("netfilter: nat: remove
l3proto struct"), but the 4.19 and below LTS branches remain vulnerable.
I have verifed the OOB kernel panic is fixed with this patch on both the
4.19 and 4.14 kernels using the approariate hardware.

Please review the fix and apply to branches 4.19.y, 4.14.y, 4.9.y and
4.4.y.

Thanks,
Will

Will McVicker (1):
  netfilter: nat: add range checks for access to nf_nat_l[34]protos[]

 net/ipv4/netfilter/nf_nat_l3proto_ipv4.c |  6 --
 net/ipv6/netfilter/nf_nat_l3proto_ipv6.c |  5 +++--
 net/netfilter/nf_nat_core.c  | 27 ++--
 net/netfilter/nf_nat_helper.c|  4 
 4 files changed, 36 insertions(+), 6 deletions(-)

-- 
2.28.0.rc0.142.g3c755180ce-goog



[PATCH 1/1] netfilter: nat: add range checks for access to nf_nat_l[34]protos[]

2020-07-27 Thread Will McVicker
The indexes to the nf_nat_l[34]protos arrays come from userspace. So we
need to make sure that before indexing the arrays, we verify the index
is within the array bounds in order to prevent an OOB memory access.
Here is an example kernel panic on 4.14.180 when userspace passes in an
index greater than NFPROTO_NUMPROTO.

Internal error: Oops - BUG: 0 [#1] PREEMPT SMP
Modules linked in:...
Process poc (pid: 5614, stack limit = 0xa3933121)
CPU: 4 PID: 5614 Comm: poc Tainted: G S  W  O4.14.180-g051355490483
Hardware name: Qualcomm Technologies, Inc. SM8150 V2 PM8150 Google Inc. MSM
task: 2a3dfffe task.stack: a3933121
pc : __cfi_check_fail+0x1c/0x24
lr : __cfi_check_fail+0x1c/0x24
...
Call trace:
__cfi_check_fail+0x1c/0x24
name_to_dev_t+0x0/0x468
nfnetlink_parse_nat_setup+0x234/0x258
ctnetlink_parse_nat_setup+0x4c/0x228
ctnetlink_new_conntrack+0x590/0xc40
nfnetlink_rcv_msg+0x31c/0x4d4
netlink_rcv_skb+0x100/0x184
nfnetlink_rcv+0xf4/0x180
netlink_unicast+0x360/0x770
netlink_sendmsg+0x5a0/0x6a4
___sys_sendmsg+0x314/0x46c
SyS_sendmsg+0xb4/0x108
el0_svc_naked+0x34/0x38

Signed-off-by: Will McVicker 
---
 net/ipv4/netfilter/nf_nat_l3proto_ipv4.c |  6 --
 net/ipv6/netfilter/nf_nat_l3proto_ipv6.c |  5 +++--
 net/netfilter/nf_nat_core.c  | 27 ++--
 net/netfilter/nf_nat_helper.c|  4 
 4 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/netfilter/nf_nat_l3proto_ipv4.c 
b/net/ipv4/netfilter/nf_nat_l3proto_ipv4.c
index 6115bf1ff6f0..1912fc66147c 100644
--- a/net/ipv4/netfilter/nf_nat_l3proto_ipv4.c
+++ b/net/ipv4/netfilter/nf_nat_l3proto_ipv4.c
@@ -218,7 +218,8 @@ int nf_nat_icmp_reply_translation(struct sk_buff *skb,
return 1;
 
l4proto = __nf_nat_l4proto_find(NFPROTO_IPV4, inside->ip.protocol);
-   if (!nf_nat_ipv4_manip_pkt(skb, hdrlen + sizeof(inside->icmp),
+   if (!l4proto ||
+   !nf_nat_ipv4_manip_pkt(skb, hdrlen + sizeof(inside->icmp),
   l4proto, &ct->tuplehash[!dir].tuple, !manip))
return 0;
 
@@ -234,7 +235,8 @@ int nf_nat_icmp_reply_translation(struct sk_buff *skb,
/* Change outer to look like the reply to an incoming packet */
nf_ct_invert_tuplepr(&target, &ct->tuplehash[!dir].tuple);
l4proto = __nf_nat_l4proto_find(NFPROTO_IPV4, 0);
-   if (!nf_nat_ipv4_manip_pkt(skb, 0, l4proto, &target, manip))
+   if (!l4proto ||
+   !nf_nat_ipv4_manip_pkt(skb, 0, l4proto, &target, manip))
return 0;
 
return 1;
diff --git a/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c 
b/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c
index ca6d38698b1a..a72840baf27b 100644
--- a/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c
+++ b/net/ipv6/netfilter/nf_nat_l3proto_ipv6.c
@@ -228,7 +228,8 @@ int nf_nat_icmpv6_reply_translation(struct sk_buff *skb,
return 1;
 
l4proto = __nf_nat_l4proto_find(NFPROTO_IPV6, inside->ip6.nexthdr);
-   if (!nf_nat_ipv6_manip_pkt(skb, hdrlen + sizeof(inside->icmp6),
+   if (!l4proto ||
+   !nf_nat_ipv6_manip_pkt(skb, hdrlen + sizeof(inside->icmp6),
   l4proto, &ct->tuplehash[!dir].tuple, !manip))
return 0;
 
@@ -245,7 +246,7 @@ int nf_nat_icmpv6_reply_translation(struct sk_buff *skb,
 
nf_ct_invert_tuplepr(&target, &ct->tuplehash[!dir].tuple);
l4proto = __nf_nat_l4proto_find(NFPROTO_IPV6, IPPROTO_ICMPV6);
-   if (!nf_nat_ipv6_manip_pkt(skb, 0, l4proto, &target, manip))
+   if (!l4proto || !nf_nat_ipv6_manip_pkt(skb, 0, l4proto, &target, manip))
return 0;
 
return 1;
diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
index 2268b10a9dcf..d28185f38955 100644
--- a/net/netfilter/nf_nat_core.c
+++ b/net/netfilter/nf_nat_core.c
@@ -64,12 +64,16 @@ struct nat_net {
 inline const struct nf_nat_l3proto *
 __nf_nat_l3proto_find(u8 family)
 {
+   if (family >= NFPROTO_NUMPROTO)
+   return NULL;
return rcu_dereference(nf_nat_l3protos[family]);
 }
 
 inline const struct nf_nat_l4proto *
 __nf_nat_l4proto_find(u8 family, u8 protonum)
 {
+   if (family >= NFPROTO_NUMPROTO)
+   return NULL;
return rcu_dereference(nf_nat_l4protos[family][protonum]);
 }
 EXPORT_SYMBOL_GPL(__nf_nat_l4proto_find);
@@ -317,7 +321,7 @@ find_best_ips_proto(const struct nf_conntrack_zone *zone,
  * range. It might not be possible to get a unique tuple, but we try.
  * At worst (or if we race), we will end up with a final duplicate in
  * __ip_conntrack_confirm and drop the packet. */
-static void
+static int
 get_unique_tuple(struct nf_conntrack_tuple *tuple,
 const struct nf_conntrack_tuple *orig_tuple,
 const struct nf_nat_range2 *range,
@@ -328,13 +332,22 @@ get_unique_tuple(struct nf_conntrack_tupl