[PATCH] drm/gma500: dont expose bytes from kernel stack

[PATCH] drm/gma500: dont expose bytes from kernel stack

[Heinrich Schuchardt]

Components m1, m2, p2, dot, vco of variable clock should be
initialized to avoid bytes from the kernel stack to be
exposed.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 drivers/gpu/drm/gma500/oaktrail_crtc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/gma500/oaktrail_crtc.c b/drivers/gpu/drm/gma500/oaktrail_crtc.c
index da9fd34..28bd8f3 100644
--- a/drivers/gpu/drm/gma500/oaktrail_crtc.c
+++ b/drivers/gpu/drm/gma500/oaktrail_crtc.c
@@ -138,6 +138,7 @@ static bool mrst_sdvo_find_best_pll(const struct gma_limit_t *limit,
 	u32 target_vco, actual_freq;
 	s32 freq_error, min_error = 100000;
 
+	memset(clock, 0, sizeof(struct gma_clock_t));
 	memset(best_clock, 0, sizeof(*best_clock));
 
 	for (clock.m = limit->m.min; clock.m <= limit->m.max; clock.m++) {
-- 
2.1.4

Re: [PATCH] drm/gma500: dont expose bytes from kernel stack

[Joe Perches]

On Sun, 2016-08-21 at 20:39 +0200, Heinrich Schuchardt wrote:
> Components m1, m2, p2, dot, vco of variable clock should be
> initialized to avoid bytes from the kernel stack to be
> exposed.

How was this found? visual code inspection?

And isn't this true for mrst_lvds_find_best_pll as well?

> a@@ -138,6 +138,7 @@ static bool mrst_sdvo_find_best_pll(const struct gma_limit_t *limit,
>  	u32 target_vco, actual_freq;
>  	s32 freq_error, min_error = 100000;
>  
> +	memset(clock, 0, sizeof(struct gma_clock_t));
>  	memset(best_clock, 0, sizeof(*best_clock));
>  
>  	for (clock.m = limit->m.min; clock.m <= limit->m.max; clock.m++) {

Re: [PATCH] drm/gma500: dont expose bytes from kernel stack

[Heinrich Schuchardt]

On 08/21/2016 08:46 PM, Joe Perches wrote:
> On Sun, 2016-08-21 at 20:39 +0200, Heinrich Schuchardt wrote:
>> Components m1, m2, p2, dot, vco of variable clock should be
>> initialized to avoid bytes from the kernel stack to be
>> exposed.
> 
> How was this found? visual code inspection?

cppcheck (http://cppcheck.sourceforge.net/)

Re: [PATCH] drm/gma500: dont expose bytes from kernel stack

[Joe Perches]

On Sun, 2016-08-21 at 21:35 +0200, Heinrich Schuchardt wrote:
> On 08/21/2016 08:46 PM, Joe Perches wrote:
> > On Sun, 2016-08-21 at 20:39 +0200, Heinrich Schuchardt wrote:
> > > Components m1, m2, p2, dot, vco of variable clock should be
> > > initialized to avoid bytes from the kernel stack to be
> > > exposed.
> > How was this found? visual code inspection?
> cppcheck (http://cppcheck.sourceforge.net/)

This should probably be mentioned in the changelog.

What about mrst_lvds_find_best_pll ?

Re: [PATCH] drm/gma500: dont expose bytes from kernel stack

[Daniel Vetter]

On Sun, Aug 21, 2016 at 08:39:38PM +0200, Heinrich Schuchardt wrote:
> Components m1, m2, p2, dot, vco of variable clock should be
> initialized to avoid bytes from the kernel stack to be
> exposed.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

Might be a silly question, but where exactly would we expose these bytes?
This isn't directly called by an ioctl, I have no idea how those bytes
might get to userspace ...
-Daniel
> ---
>  drivers/gpu/drm/gma500/oaktrail_crtc.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/gma500/oaktrail_crtc.c b/drivers/gpu/drm/gma500/oaktrail_crtc.c
> index da9fd34..28bd8f3 100644
> --- a/drivers/gpu/drm/gma500/oaktrail_crtc.c
> +++ b/drivers/gpu/drm/gma500/oaktrail_crtc.c
> @@ -138,6 +138,7 @@ static bool mrst_sdvo_find_best_pll(const struct gma_limit_t *limit,
>  	u32 target_vco, actual_freq;
>  	s32 freq_error, min_error = 100000;
>  
> +	memset(clock, 0, sizeof(struct gma_clock_t));
>  	memset(best_clock, 0, sizeof(*best_clock));
>  
>  	for (clock.m = limit->m.min; clock.m <= limit->m.max; clock.m++) {
> -- 
> 2.1.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH] drm/gma500: dont expose bytes from kernel stack

[Jani Nikula]

On Sun, 21 Aug 2016, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> Components m1, m2, p2, dot, vco of variable clock should be
> initialized to avoid bytes from the kernel stack to be
> exposed.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  drivers/gpu/drm/gma500/oaktrail_crtc.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/gma500/oaktrail_crtc.c b/drivers/gpu/drm/gma500/oaktrail_crtc.c
> index da9fd34..28bd8f3 100644
> --- a/drivers/gpu/drm/gma500/oaktrail_crtc.c
> +++ b/drivers/gpu/drm/gma500/oaktrail_crtc.c
> @@ -138,6 +138,7 @@ static bool mrst_sdvo_find_best_pll(const struct gma_limit_t *limit,
>  	u32 target_vco, actual_freq;
>  	s32 freq_error, min_error = 100000;
>  
> +	memset(clock, 0, sizeof(struct gma_clock_t));

Did you build this? Did you run this?

BR,
Jani.


>  	memset(best_clock, 0, sizeof(*best_clock));
>  
>  	for (clock.m = limit->m.min; clock.m <= limit->m.max; clock.m++) {

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH] drm/gma500: dont expose bytes from kernel stack

[One Thousand Gnomes]

On Mon, 22 Aug 2016 09:29:17 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Sun, Aug 21, 2016 at 08:39:38PM +0200, Heinrich Schuchardt wrote:
> > Components m1, m2, p2, dot, vco of variable clock should be
> > initialized to avoid bytes from the kernel stack to be
> > exposed.
> > 
> > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>  
> 
> Might be a silly question, but where exactly would we expose these bytes?
> This isn't directly called by an ioctl, I have no idea how those bytes
> might get to userspace ...

mrst_print_pll displays clock.p2 - which is indeed never cleared 8)

Alan

Re: [PATCH] drm/gma500: dont expose bytes from kernel stack

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2298 bytes --]

Hi Heinrich,

[auto build test ERROR on drm/drm-next]
[also build test ERROR on v4.8-rc3 next-20160822]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Heinrich-Schuchardt/drm-gma500-dont-expose-bytes-from-kernel-stack/20160822-024132
base:   git://people.freedesktop.org/~airlied/linux.git drm-next
config: x86_64-randconfig-x015-201634 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/gpu/drm/gma500/oaktrail_crtc.c: In function 'mrst_sdvo_find_best_pll':
>> drivers/gpu/drm/gma500/oaktrail_crtc.c:141:9: error: incompatible type for argument 1 of 'memset'
     memset(clock, 0, sizeof(struct gma_clock_t));
            ^~~~~
   In file included from arch/x86/include/asm/string.h:4:0,
                    from include/linux/string.h:18,
                    from include/uapi/linux/uuid.h:21,
                    from include/linux/uuid.h:19,
                    from include/linux/mod_devicetable.h:12,
                    from include/linux/i2c.h:29,
                    from drivers/gpu/drm/gma500/oaktrail_crtc.c:18:
   arch/x86/include/asm/string_64.h:55:7: note: expected 'void *' but argument is of type 'struct gma_clock_t'
    void *memset(void *s, int c, size_t n);
          ^~~~~~

vim +/memset +141 drivers/gpu/drm/gma500/oaktrail_crtc.c

   135					    int refclk, struct gma_clock_t *best_clock)
   136	{
   137		struct gma_clock_t clock;
   138		u32 target_vco, actual_freq;
   139		s32 freq_error, min_error = 100000;
   140	
 > 141		memset(clock, 0, sizeof(struct gma_clock_t));
   142		memset(best_clock, 0, sizeof(*best_clock));
   143	
   144		for (clock.m = limit->m.min; clock.m <= limit->m.max; clock.m++) {

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 31106 bytes --]

Re: [PATCH] drm/gma500: dont expose bytes from kernel stack

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 7414 bytes --]

Hi Heinrich,

[auto build test ERROR on drm/drm-next]
[also build test ERROR on v4.8-rc3 next-20160822]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
[Suggest to use git(>=2.9.0) format-patch --base=<commit> (or --base=auto for convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:    https://github.com/0day-ci/linux/commits/Heinrich-Schuchardt/drm-gma500-dont-expose-bytes-from-kernel-stack/20160822-024132
base:   git://people.freedesktop.org/~airlied/linux.git drm-next
config: i386-randconfig-i0-08220007 (attached as .config)
compiler: gcc-4.8 (Debian 4.8.4-1) 4.8.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   In file included from arch/x86/include/asm/string.h:2:0,
                    from include/linux/string.h:18,
                    from include/uapi/linux/uuid.h:21,
                    from include/linux/uuid.h:19,
                    from include/linux/mod_devicetable.h:12,
                    from include/linux/i2c.h:29,
                    from drivers/gpu/drm/gma500/oaktrail_crtc.c:18:
   drivers/gpu/drm/gma500/oaktrail_crtc.c: In function 'mrst_sdvo_find_best_pll':
>> drivers/gpu/drm/gma500/oaktrail_crtc.c:141:33: error: incompatible type for argument 1 of '__builtin_memset'
     memset(clock, 0, sizeof(struct gma_clock_t));
                                    ^
   arch/x86/include/asm/string_32.h:325:52: note: in definition of macro 'memset'
    #define memset(s, c, count) __builtin_memset(s, c, count)
                                                       ^
   drivers/gpu/drm/gma500/oaktrail_crtc.c:141:33: note: expected 'void *' but argument is of type 'struct gma_clock_t'
     memset(clock, 0, sizeof(struct gma_clock_t));
                                    ^
   arch/x86/include/asm/string_32.h:325:52: note: in definition of macro 'memset'
    #define memset(s, c, count) __builtin_memset(s, c, count)
                                                       ^

vim +/__builtin_memset +141 drivers/gpu/drm/gma500/oaktrail_crtc.c

    12	 *
    13	 * You should have received a copy of the GNU General Public License along with
    14	 * this program; if not, write to the Free Software Foundation, Inc.,
    15	 * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
    16	 */
    17	
  > 18	#include <linux/i2c.h>
    19	#include <linux/pm_runtime.h>
    20	
    21	#include <drm/drmP.h>
    22	#include "framebuffer.h"
    23	#include "psb_drv.h"
    24	#include "psb_intel_drv.h"
    25	#include "psb_intel_reg.h"
    26	#include "gma_display.h"
    27	#include "power.h"
    28	
    29	#define MRST_LIMIT_LVDS_100L	0
    30	#define MRST_LIMIT_LVDS_83	1
    31	#define MRST_LIMIT_LVDS_100	2
    32	#define MRST_LIMIT_SDVO		3
    33	
    34	#define MRST_DOT_MIN		  19750
    35	#define MRST_DOT_MAX		  120000
    36	#define MRST_M_MIN_100L		    20
    37	#define MRST_M_MIN_100		    10
    38	#define MRST_M_MIN_83		    12
    39	#define MRST_M_MAX_100L		    34
    40	#define MRST_M_MAX_100		    17
    41	#define MRST_M_MAX_83		    20
    42	#define MRST_P1_MIN		    2
    43	#define MRST_P1_MAX_0		    7
    44	#define MRST_P1_MAX_1		    8
    45	
    46	static bool mrst_lvds_find_best_pll(const struct gma_limit_t *limit,
    47					    struct drm_crtc *crtc, int target,
    48					    int refclk, struct gma_clock_t *best_clock);
    49	
    50	static bool mrst_sdvo_find_best_pll(const struct gma_limit_t *limit,
    51					    struct drm_crtc *crtc, int target,
    52					    int refclk, struct gma_clock_t *best_clock);
    53	
    54	static const struct gma_limit_t mrst_limits[] = {
    55		{			/* MRST_LIMIT_LVDS_100L */
    56		 .dot = {.min = MRST_DOT_MIN, .max = MRST_DOT_MAX},
    57		 .m = {.min = MRST_M_MIN_100L, .max = MRST_M_MAX_100L},
    58		 .p1 = {.min = MRST_P1_MIN, .max = MRST_P1_MAX_1},
    59		 .find_pll = mrst_lvds_find_best_pll,
    60		 },
    61		{			/* MRST_LIMIT_LVDS_83L */
    62		 .dot = {.min = MRST_DOT_MIN, .max = MRST_DOT_MAX},
    63		 .m = {.min = MRST_M_MIN_83, .max = MRST_M_MAX_83},
    64		 .p1 = {.min = MRST_P1_MIN, .max = MRST_P1_MAX_0},
    65		 .find_pll = mrst_lvds_find_best_pll,
    66		 },
    67		{			/* MRST_LIMIT_LVDS_100 */
    68		 .dot = {.min = MRST_DOT_MIN, .max = MRST_DOT_MAX},
    69		 .m = {.min = MRST_M_MIN_100, .max = MRST_M_MAX_100},
    70		 .p1 = {.min = MRST_P1_MIN, .max = MRST_P1_MAX_1},
    71		 .find_pll = mrst_lvds_find_best_pll,
    72		 },
    73		{			/* MRST_LIMIT_SDVO */
    74		 .vco = {.min = 1400000, .max = 2800000},
    75		 .n = {.min = 3, .max = 7},
    76		 .m = {.min = 80, .max = 137},
    77		 .p1 = {.min = 1, .max = 2},
    78		 .p2 = {.dot_limit = 200000, .p2_slow = 10, .p2_fast = 10},
    79		 .find_pll = mrst_sdvo_find_best_pll,
    80		 },
    81	};
    82	
    83	#define MRST_M_MIN	    10
    84	static const u32 oaktrail_m_converts[] = {
    85		0x2B, 0x15, 0x2A, 0x35, 0x1A, 0x0D, 0x26, 0x33, 0x19, 0x2C,
    86		0x36, 0x3B, 0x1D, 0x2E, 0x37, 0x1B, 0x2D, 0x16, 0x0B, 0x25,
    87		0x12, 0x09, 0x24, 0x32, 0x39, 0x1c,
    88	};
    89	
    90	static const struct gma_limit_t *mrst_limit(struct drm_crtc *crtc,
    91						    int refclk)
    92	{
    93		const struct gma_limit_t *limit = NULL;
    94		struct drm_device *dev = crtc->dev;
    95		struct drm_psb_private *dev_priv = dev->dev_private;
    96	
    97		if (gma_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)
    98		    || gma_pipe_has_type(crtc, INTEL_OUTPUT_MIPI)) {
    99			switch (dev_priv->core_freq) {
   100			case 100:
   101				limit = &mrst_limits[MRST_LIMIT_LVDS_100L];
   102				break;
   103			case 166:
   104				limit = &mrst_limits[MRST_LIMIT_LVDS_83];
   105				break;
   106			case 200:
   107				limit = &mrst_limits[MRST_LIMIT_LVDS_100];
   108				break;
   109			}
   110		} else if (gma_pipe_has_type(crtc, INTEL_OUTPUT_SDVO)) {
   111			limit = &mrst_limits[MRST_LIMIT_SDVO];
   112		} else {
   113			limit = NULL;
   114			dev_err(dev->dev, "mrst_limit Wrong display type.\n");
   115		}
   116	
   117		return limit;
   118	}
   119	
   120	/** Derive the pixel clock for the given refclk and divisors for 8xx chips. */
   121	static void mrst_lvds_clock(int refclk, struct gma_clock_t *clock)
   122	{
   123		clock->dot = (refclk * clock->m) / (14 * clock->p1);
   124	}
   125	
   126	static void mrst_print_pll(struct gma_clock_t *clock)
   127	{
   128		DRM_DEBUG_DRIVER("dotclock=%d,  m=%d, m1=%d, m2=%d, n=%d, p1=%d, p2=%d\n",
   129				 clock->dot, clock->m, clock->m1, clock->m2, clock->n,
   130				 clock->p1, clock->p2);
   131	}
   132	
   133	static bool mrst_sdvo_find_best_pll(const struct gma_limit_t *limit,
   134					    struct drm_crtc *crtc, int target,
   135					    int refclk, struct gma_clock_t *best_clock)
   136	{
   137		struct gma_clock_t clock;
   138		u32 target_vco, actual_freq;
   139		s32 freq_error, min_error = 100000;
   140	
 > 141		memset(clock, 0, sizeof(struct gma_clock_t));
   142		memset(best_clock, 0, sizeof(*best_clock));
   143	
   144		for (clock.m = limit->m.min; clock.m <= limit->m.max; clock.m++) {

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 25540 bytes --]