Message ID | 1444976055-19148-1-git-send-email-christophe.jaillet@wanadoo.fr (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
Hi Christophe, [auto build test ERROR on powerpc/next -- if it's inappropriate base, please suggest rules for selecting the more suitable base] url: https://github.com/0day-ci/linux/commits/Christophe-JAILLET/powerpc-prom-Avoid-reference-to-potentially-freed-memory/20151016-141714 config: powerpc-defconfig (attached as .config) reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree make.cross ARCH=powerpc All errors (new ones prefixed by >>): arch/powerpc/kernel/prom.c: In function 'of_get_ibm_chip_id': >> arch/powerpc/kernel/prom.c:787:23: error: unused variable 'old' [-Werror=unused-variable] struct device_node *old = np; ^ >> arch/powerpc/kernel/prom.c:795:15: error: 'old' undeclared (first use in this function) of_node_put(old); ^ arch/powerpc/kernel/prom.c:795:15: note: each undeclared identifier is reported only once for each function it appears in arch/powerpc/kernel/prom.c: At top level: >> arch/powerpc/kernel/prom.c:797:2: error: expected identifier or '(' before 'return' return -1; ^ >> arch/powerpc/kernel/prom.c:798:1: error: expected identifier or '(' before '}' token } ^ arch/powerpc/kernel/prom.c: In function 'of_get_ibm_chip_id': arch/powerpc/kernel/prom.c:796:2: error: control reaches end of non-void function [-Werror=return-type] } ^ cc1: all warnings being treated as errors vim +/old +787 arch/powerpc/kernel/prom.c b37193b7 Benjamin Herrenschmidt 2013-07-15 781 * be found. b37193b7 Benjamin Herrenschmidt 2013-07-15 782 */ b37193b7 Benjamin Herrenschmidt 2013-07-15 783 int of_get_ibm_chip_id(struct device_node *np) b37193b7 Benjamin Herrenschmidt 2013-07-15 784 { b37193b7 Benjamin Herrenschmidt 2013-07-15 785 of_node_get(np); b37193b7 Benjamin Herrenschmidt 2013-07-15 786 while (np) { b37193b7 Benjamin Herrenschmidt 2013-07-15 @787 struct device_node *old = np; 12540384 Christophe JAILLET 2015-10-16 788 u32 chip_id; b37193b7 Benjamin Herrenschmidt 2013-07-15 789 12540384 Christophe JAILLET 2015-10-16 790 if (!of_property_read_u32(np, "ibm,chip-id", &chip_id)) b37193b7 Benjamin Herrenschmidt 2013-07-15 791 of_node_put(np); 12540384 Christophe JAILLET 2015-10-16 792 return chip_id; b37193b7 Benjamin Herrenschmidt 2013-07-15 793 } b37193b7 Benjamin Herrenschmidt 2013-07-15 794 np = of_get_parent(np); b37193b7 Benjamin Herrenschmidt 2013-07-15 @795 of_node_put(old); b37193b7 Benjamin Herrenschmidt 2013-07-15 796 } b37193b7 Benjamin Herrenschmidt 2013-07-15 @797 return -1; b37193b7 Benjamin Herrenschmidt 2013-07-15 @798 } b130e7c0 Dan Streetman 2015-05-07 799 EXPORT_SYMBOL(of_get_ibm_chip_id); b37193b7 Benjamin Herrenschmidt 2013-07-15 800 3eb906c6 Michael Ellerman 2013-11-20 801 /** :::::: The code at line 787 was first introduced by commit :::::: b37193b71846858d816e152d3a5db010d7b73f5e powerpc/powernv: Add helper to get ibm,chip-id of a node :::::: TO: Benjamin Herrenschmidt <benh@kernel.crashing.org> :::::: CC: Benjamin Herrenschmidt <benh@kernel.crashing.org> --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Fri, 2015-10-16 at 08:14 +0200, Christophe JAILLET wrote: > of_get_property() is used inside the loop, but then the reference to the > node is dropped before dereferencing the prop pointer, which could by then > point to junk if the node has been freed. > > Instead use of_property_read_u32() to actually read the property > value before dropping the reference. > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > *** UNTESTED *** > --- > arch/powerpc/kernel/prom.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c > index bef76c5..dc4f6a4 100644 > --- a/arch/powerpc/kernel/prom.c > +++ b/arch/powerpc/kernel/prom.c > @@ -783,14 +783,13 @@ void __init early_get_first_memblock_info(void *params, phys_addr_t *size) > int of_get_ibm_chip_id(struct device_node *np) > { > of_node_get(np); > - while(np) { > + while (np) { > struct device_node *old = np; > - const __be32 *prop; > + u32 chip_id; > > - prop = of_get_property(np, "ibm,chip-id", NULL); > - if (prop) { > + if (!of_property_read_u32(np, "ibm,chip-id", &chip_id)) > of_node_put(np); > - return be32_to_cpup(prop); > + return chip_id; > } As the kbuild robot detected you have left an extra "}" here. I don't mind too much if you send patches that aren't compile tested, but you might save yourself some time by compiling them. There are x86->powerpc cross compilers here: https://www.kernel.org/pub/tools/crosstool/files/bin/x86_64/4.9.0/x86_64-gcc-4.9.0-nolibc_powerpc64-linux.tar.gz Or if you're running on Ubuntu you can just do: $ apt-get install gcc-powerpc-linux-gnu I think there's a package for Fedora too but I don't know the name off the top of my head. cheers
Le 16/10/2015 12:02, Michael Ellerman a écrit : > As the kbuild robot detected you have left an extra "}" here. > > I don't mind too much if you send patches that aren't compile tested, but you > might save yourself some time by compiling them. Sorry about it, and thanks for your patience. IMHO, this should never happen and patches should be at least compile-tested. I will be more careful and compile-test any new patch I submit. CJ
On Fri, 2015-10-16 at 22:09 +0200, Christophe JAILLET wrote: > Le 16/10/2015 12:02, Michael Ellerman a écrit : > > As the kbuild robot detected you have left an extra "}" here. > > > > I don't mind too much if you send patches that aren't compile tested, but you > > might save yourself some time by compiling them. > > Sorry about it, and thanks for your patience. > IMHO, this should never happen and patches should be at least > compile-tested. Preferably yes. But for these sort of cleanup patches, where you're touching lots of different arches, I realise it can be tedious to find all the various cross compilers. So I'm willing to cut you a bit of slack :) > I will be more careful and compile-test any new patch I submit. Thanks. cheers
diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c index bef76c5..dc4f6a4 100644 --- a/arch/powerpc/kernel/prom.c +++ b/arch/powerpc/kernel/prom.c @@ -783,14 +783,13 @@ void __init early_get_first_memblock_info(void *params, phys_addr_t *size) int of_get_ibm_chip_id(struct device_node *np) { of_node_get(np); - while(np) { + while (np) { struct device_node *old = np; - const __be32 *prop; + u32 chip_id; - prop = of_get_property(np, "ibm,chip-id", NULL); - if (prop) { + if (!of_property_read_u32(np, "ibm,chip-id", &chip_id)) of_node_put(np); - return be32_to_cpup(prop); + return chip_id; } np = of_get_parent(np); of_node_put(old);
of_get_property() is used inside the loop, but then the reference to the node is dropped before dereferencing the prop pointer, which could by then point to junk if the node has been freed. Instead use of_property_read_u32() to actually read the property value before dropping the reference. Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> --- *** UNTESTED *** --- arch/powerpc/kernel/prom.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)